-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Add nodePort declaration to chart/values.schema.json #26945
Add nodePort declaration to chart/values.schema.json #26945
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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
Do you think you could add a test for this? |
@dstandish just added one and tested locally. Let me know if it needs more coverage |
0ebfafe
to
de9562e
Compare
) | ||
] | ||
) | ||
def test_nodeport_service(self, ports, expected_ports): |
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.
looks like your params aren't consistent with those provided in expand
but let's see what the tests tell us
i'd also add an entry (in expand) for when no nodePort is provided
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.
looks like your params aren't consistent with those provided in expand
@dstandish Not sure I follow-- I used the test_ports_overrides param structure as a reference, and it looks like CI checks are passing
i'd also add an entry (in expand) for when no nodePort is provided
Added (lines 730-733) and tested locally with success. We'll see if the remote CI checks pass as well
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.
all good.
one thing though.
looking here, it looks like name
is not a valid attr for nodeport. probably best to remov that.
b031585
to
52d3aa9
Compare
44a5a0b
to
24e04a7
Compare
@dstandish would you be able to re-review this this week? No worries if not, just checking. It also looks like I somehow set you as the only reviewer, so if there's someone else who can review it as well, let me know |
047ca0c
to
75a38eb
Compare
^ added a trailing comma that was causing the Black CI check to fail. Tests can be re-run when possible |
23162de
to
ee252ad
Compare
ee252ad
to
41feda7
Compare
…t setting of a nodePort port value when deploying via Helm chart
…till specified as NodePort
41feda7
to
6f1b237
Compare
I run the workflow. @dstandish ? |
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.
small things
) | ||
] | ||
) | ||
def test_nodeport_service(self, ports, expected_ports): |
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.
all good.
one thing though.
looking here, it looks like name
is not a valid attr for nodeport. probably best to remov that.
Co-authored-by: Daniel Standish <15932138+dstandish@users.noreply.github.com>
Co-authored-by: Daniel Standish <15932138+dstandish@users.noreply.github.com>
Thanks @dstandish and @potiuk! @dstandish I've reviewed and committed your recommended changes |
Awesome work, congrats on your first merged pull request! |
closes: #26812
Adding a nodePort declaration to the chart/values.schema.json file to allow users to explicitly define exactly which port the nodePort setting should use when deploying with the Helm chart.
This is important for users who have external load balancers and firewalls/security groups that have predefined traffic ports for Airflow. (e.g. a load balancer is expecting the airflow-webserver to listen on port 31000, and it's set up via Terraform, so I'd rather be able to specify a known port in Terraform beforehand rather than have to grab a new port every time I build Airflow)
Tested locally on minikube with the following values.yaml override.
Cluster comes up successfully with airflow-webserver service listening on expected nodePort. Output of the deployed
airflow-webserver
service object's YAML below:Also tested without specifying a nodePort option (defaulting to ClusterIP) to ensure new schema doesn't break backwards compatibility.