Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Respect deployment labels in the CSV #5533

Merged
merged 1 commit into from
Mar 1, 2022
Merged

Respect deployment labels in the CSV #5533

merged 1 commit into from
Mar 1, 2022

Conversation

creydr
Copy link
Contributor

@creydr creydr commented Feb 2, 2022 •
edited
Loading

Description of the change:
This change adds the labels of the deployment to the ClusterServiceVersions DeploymentSpecs. So the labels defined for the deployment (and defined in its manifest) will be applied too, if the operator gets bundled and installed via OLM.

Motivation for the change:
Currently the CSV does not contain the labels of the deployment in the deployment spec, even there is a field for it (clusterserviceversion_types.go#L71).
This leads to missing labels of the deployment, when the operator is bundled and installed via OLM. This PR addresses it and adds the deployment labels to the CSV.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@creydr
Copy link
Contributor Author

creydr commented Feb 2, 2022

/assign @camilamacedo86
/assign @varshaprasad96

@creydr
Copy link
Contributor Author

creydr commented Feb 9, 2022

@camilamacedo86 the "sanity / docs" job seemed to have some connection issues. Is there a way to retrigger this?

@creydr
Copy link
Contributor Author

creydr commented Feb 10, 2022

@fabianvf can you help me with this?

varshaprasad96
varshaprasad96 reviewed Feb 14, 2022
View reviewed changes
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the one issue of adding webhookPath in samples, rest seem fine.

internal/generate/testdata/clusterserviceversions/output/memcached-operator-multiVersion.yaml
webhookPath: /validate-cache-example-com-v1alpha1-memcached
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why does the webhook path get added? Webhookdefinitions are defined in operator-framework/api

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only added a newline at the end of the file. Same as with https://github.com/operator-framework/operator-sdk/pull/5533/files#diff-bd889e1aa1c1539a692594a301ce58562dd1f903280a9c0a324d8503a2f8b1eaR212.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my bad. Overlooked it.

@creydr
Copy link
Contributor Author

creydr commented Feb 15, 2022

Apart from the one issue of adding webhookPath in samples, rest seem fine.

Thanks @varshaprasad96 for having a review on it. I added comments to your findings. For sure I can change it as well...

@creydr creydr force-pushed the respect-deployment-labels-in-csv branch from 239702a to 288c8bc Compare February 25, 2022 10:17
camilamacedo86
camilamacedo86 approved these changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. 🥇
It shows fine for me

/approved

I think would be nice we get +1 reviewer before it gets merged. (to lgtm)

@creydr
Respect deployment labels in the CSV
bd079cd 
This change adds the labels of the deployment to the
ClusterServiceVersions deployment spec. So the labels defined for the
deployment (and defined in its manifest) will be applied too, if the
operator gets installed via OLM.

Signed-off-by: Christoph Stäbler <[email protected]>
@creydr creydr force-pushed the respect-deployment-labels-in-csv branch from 288c8bc to bd079cd Compare February 25, 2022 10:49
varshaprasad96
varshaprasad96 approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me to! Thanks!
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2022
@creydr
Copy link
Contributor Author

creydr commented Feb 27, 2022

/retest-required

@openshift-ci
Copy link

openshift-ci bot commented Feb 27, 2022

@creydr: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest-required

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@creydr
Copy link
Contributor Author

creydr commented Feb 27, 2022

@camilamacedo86 @varshaprasad96 thanks for your review and lgtm.
The ansible e2e failed (not sure if it is a flake, as the error message got truncated). Would it be possible to retrigger it to get this PR merged?

rashmigottipati
rashmigottipati approved these changes Feb 28, 2022
View reviewed changes
Copy link
Member

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@rashmigottipati rashmigottipati merged commit 5b118b7 into operator-framework:master Mar 1, 2022
@wallrj wallrj mentioned this pull request Apr 28, 2022
Closed
@wallrj wallrj mentioned this pull request May 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rashmigottipati rashmigottipati rashmigottipati approved these changes

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@varshaprasad96 varshaprasad96 varshaprasad96 approved these changes

Assignees

@rashmigottipati rashmigottipati

@camilamacedo86 camilamacedo86

@varshaprasad96 varshaprasad96

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@creydr @rashmigottipati @camilamacedo86 @varshaprasad96