You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "manuel-lang (via GitHub)" <gi...@apache.org> on 2023/06/29 10:16:04 UTC
[GitHub] [airflow] manuel-lang opened a new pull request, #32257: Add microsoft.mssql extra to Dockerfile
manuel-lang opened a new pull request, #32257:
URL: https://github.com/apache/airflow/pull/32257
Adding the extra package `microsoft.mssql` to the AIRFLOW_EXTRAS being install in the regular Docker image.
MSSQL is quite commonly used and tbh I don't see any reason why other extras should be included and MSSQL should not be. This proposed changed is indeed open for discussion and I will be happy to integrate additional changes if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32257: Add microsoft.mssql extra to Dockerfile
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32257:
URL: https://github.com/apache/airflow/pull/32257#issuecomment-1613579919
> Maybe this is something to be aware of. Also sqlcmd works just fine on ARM architectures.
Sure - maybe we should use this one instead? PRs are welcome :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #32257: Add microsoft.mssql extra to Dockerfile
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #32257:
URL: https://github.com/apache/airflow/pull/32257#issuecomment-1612787603
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32257: Add microsoft.mssql extra to Dockerfile
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32257:
URL: https://github.com/apache/airflow/pull/32257#issuecomment-1612950323
Note - it's not a "hard" requirement, it's more of a convenience for our users to also support development workflows for ARM image, but I think an important one for us, because I think that might be the first questions some users will ask ("Hey the image there supports mssql, and I wanted to install mssql-cli and it does not work for my ARM images - what do I do?) and we need to have either a good answer or make it works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32257: Add microsoft.mssql extra to Dockerfile
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32257:
URL: https://github.com/apache/airflow/pull/32257#issuecomment-1612942463
I would be rather against that at least not until someone works on upgrading the mssql suport for ARM image.
There are (were) various problems with MSSQL mostly related to not supporting all architectures and operating system - including the fact that for now we are excluding mssql-client from installation on ARM architecture https://github.com/apache/airflow/blob/main/scripts/docker/install_pipx_tools.sh#L27
This means that users of the ARM image will not be able to connect to the mssql database using client. Possibly this issu has been solved - but fixing it should be the first step.
Do you have access to ARM/M1 to test it and make a fix to not exclude mssql-cli from installation and make such PR @manuel-lang ? Should be as easy as running `breeze prod-image build` after making the change to exclude ARM and running pre-commit (to regenerate scripts embedded in Dockerfiles).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] manuel-lang commented on pull request #32257: Add microsoft.mssql extra to Dockerfile
Posted by "manuel-lang (via GitHub)" <gi...@apache.org>.
manuel-lang commented on PR #32257:
URL: https://github.com/apache/airflow/pull/32257#issuecomment-1613245612
Thank you a lot for your detailed explanation, @potiuk. This makes a lot of sense. I have access to an ARM device and will check the support and share my findings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] manuel-lang commented on pull request #32257: Add microsoft.mssql extra to Dockerfile
Posted by "manuel-lang (via GitHub)" <gi...@apache.org>.
manuel-lang commented on PR #32257:
URL: https://github.com/apache/airflow/pull/32257#issuecomment-1613510911
> I would be rather against that at least not until someone works on upgrading the mssql suport for ARM image.
>
> There are (were) various problems with MSSQL mostly related to not supporting all architectures and operating system - including the fact that for now we are excluding mssql-client from installation on ARM architecture https://github.com/apache/airflow/blob/main/scripts/docker/install_pipx_tools.sh#L27
>
> This means that users of the ARM image will not be able to connect to the mssql database using client. Possibly this issu has been solved - but fixing it should be the first step.
>
> Do you have access to ARM/M1 to test it and make a fix to not exclude mssql-cli from installation and make such PR @manuel-lang ? Should be as easy as running `breeze ci-image build` after making the change to exclude ARM and running pre-commit (to regenerate scripts embedded in Dockerfiles).
As written by @potiuk, mssql-cli is not supported on ARM devices. I just checked it and it still is not. So based on the explanation, I'll just close this MR.
However, [mssql-cli](https://github.com/dbcli/mssql-cli) now is (or will be) deprecated in favor of [gosql-cmd](https://learn.microsoft.com/de-de/sql/tools/sqlcmd/go-sqlcmd-utility?view=sql-server-ver16&tabs=windows). Copy of their deprecation notice:
> DEPRECATION NOTICE mssql-cli is on the path to deprecation, and will be fully replaced by the new [go-sqlcmd](https://learn.microsoft.com/sql/tools/sqlcmd/go-sqlcmd-utility) utility once it becomes generally available. We are actively in development for the new sqlcmd, and would love to hear feedback on it [here](https://github.com/microsoft/go-sqlcmd/issues)!
Maybe this is something to be aware of. Also sqlcmd works just fine on ARM architectures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] manuel-lang closed pull request #32257: Add microsoft.mssql extra to Dockerfile
Posted by "manuel-lang (via GitHub)" <gi...@apache.org>.
manuel-lang closed pull request #32257: Add microsoft.mssql extra to Dockerfile
URL: https://github.com/apache/airflow/pull/32257
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org