-
Notifications
You must be signed in to change notification settings - Fork 14.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
Chart: Allow overriding redis's runAsUser
option (#19681)
#19689
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)
|
@@ -2856,6 +2856,11 @@ | |||
"type": "array", | |||
"default": [] | |||
}, | |||
"uid": { |
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.
I believe this should come as .Values.redis.uid
parameter ?
For compatibiity with previous version also I tihnk this should have None
default and the whole security-context
section should be conditional based on the uid set, I think because someone upgrading the chart will already have a root-owned db in /data abwhen persistence is enabled and it might get non-accessible if "runAsUser" is used.
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.
Of course .Values.redis.uid
, that was an oversight.
I see the issue with upgrading with existing redis data. What do you think of the solution below?
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 good. One more thing - we also have unit tests for the chart (chart/tests) - they are rather simple - you pass parameters and check if the rendered templates are as expected. It would be great to add tests for this change there.
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.
And I also recommend installing pre-commit - I think it will then generate documentation automatically from the schema (see https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#pre-commit-hooks)
This will avoid unnecessary CI iterations.
This should probably be added after #18249 is merged so we have one less deprecation to deal with. |
Sorry - needs rebase again (but we had some intermittent problems that should nearly be gone - so please - rebase). |
c357572
to
2c1c5e1
Compare
Co-authored-by: Kaxil Naik <[email protected]>
runAsUser
option (#19681)
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
@tomasgatial, #18249 has been merged. Can you refactor this to support both |
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.
Now that #18249 has been merged, we need to refactor this to support a securityContext instead, and optionally an explicit uid.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
closes: #19681
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.