-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Fix missing api server ingress #49727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix missing api server ingress #49727
Conversation
|
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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
|
@paolofacchinetti Could you add some simple tests similar to https://github.com/apache/airflow/blob/main/helm-tests/tests/helm_tests/webserver/test_ingress_web.py ? Let me know if you need any help |
|
@kaxil done, since the new apiserver ingress is pretty much the same as the old webserver, I've just copied the tests and updated them. Let me know if that's enough. tests run successfully with |
Head branch was pushed to by a user without write access
|
@kaxil had to break a comment into 2 lines to pass linting, all other CI jobs lgtm - let me know if theres anything else to be done and thanks for the review! |
|
When testing, it was found that an addition related to webserver-config is necessary. cc. @kaxil |
|
@peter-cheon I'm not sure what you mean with "an addition related to webserver-config is necessary" and how it is related to the api-server ingress... is it something that can be done in another PR? |
Could you create a separate PR for that? |
|
@paolofacchinetti I rebased your PR and fixed static checks in fa2b400 -- hopefully tests should pass now |
|
One test failed but it is unrelated failure and known one. I'll retrigger it. |
jscheffl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Green... should we merge?
What I meant is that I will create a separate PR after the above PR has been merged. |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
closes: #49698
Since the webserver component is deprecated in Airflow 3.x.x, the existing ingress is not templated during chart deployment.
This PR adds an Ingress resource for the Airflow API Server in the Airflow Helm Chart.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.