-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat(openshift): custom extra labels/annotations on deployments/pods #632
Conversation
/build_test |
Ah right, that CI command is currently not working: #619 (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.
Hi @OpenGuidou, thank you for your contribution! Excellent work as well. I have some comments below, but more generally, do you think it makes sense to have a separate set of labels and annotations for the reports deployment (if enabled)? Typically, we'd put any properties that affect the reports deployment under the ReportOptions property.
fe942bb
to
7e17f8b
Compare
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 think this looks good. @ebaron would you want the ingresses/services/routes to also have customizable labels/annotations like this or are you OK with it just going as far as pod/deployment for now?
For that one, it's already possible via https://github.com/cryostatio/cryostat-operator/blob/main/api/v1beta1/cryostat_types.go#L226 and https://github.com/cryostatio/cryostat-operator/blob/main/api/v1beta1/cryostat_types.go#L292 |
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.
@OpenGuidou looks good! Just noticed one more thing. Could you run make bundle
to regenerate the CSV to include these new properties? We recently upgraded to Operator SDK 1.31.0, so if you have another version installed locally you'll need to upgrade.
Ha I missed that, it's now done |
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.
Excellent work @OpenGuidou! Thank you very much for the contribution!
@OpenGuidou Sorry one more thing. We require that all commits be signed in order for me to merge this PR: |
… pods Signed-off-by: Guillaume Doussin <[email protected]>
Done ! |
Welcome to Cryostat! 👋
Before contributing, make sure you have:
main
branch[chore, ci, docs, feat, fix, test]
git commit -S -m "YOUR_COMMIT_MESSAGE"
Fixes: #630
Description of the change:
This change adds allows the users to provide additional labels and annotations to be used in the cryostat deployment and pod.
Motivation for the change:
This change is helpful because some environment have a strong labeling policy for all resources.