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

Allow removing version from Kubernetes label selectors #11661

Merged
merged 3 commits into from
Sep 23, 2020
Merged

Allow removing version from Kubernetes label selectors #11661

merged 3 commits into from
Sep 23, 2020

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Aug 27, 2020

Fixes: #11070

@geoand geoand requested a review from iocanel August 27, 2020 09:34
@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 27, 2020
@gsmet gsmet changed the title Allow removing version from k8s label selectors Allow removing version from Kubernetes label selectors Aug 27, 2020
@geoand geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 27, 2020
@geoand
Copy link
Contributor Author

geoand commented Aug 28, 2020

@iocanel can you take a look please?

@gsmet
Copy link
Member

gsmet commented Aug 31, 2020

This needs a rebase.

@gsmet gsmet added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Aug 31, 2020
@geoand
Copy link
Contributor Author

geoand commented Aug 31, 2020

Done, @iocanel can you please review?

@geoand geoand removed the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Aug 31, 2020
@gsmet
Copy link
Member

gsmet commented Aug 31, 2020

@iocanel could you review this one tomorrow? We need it in CR1 IMHO. Thanks.

iocanel
iocanel requested changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

We don't need decorator per resource. We can just create a single decorator that will be applied everywhere. Maybe a decorator on ObjectMetaFluent is all we need. An alternative would be on HasMetadataFluent or something

@geoand
Copy link
Contributor Author

geoand commented Sep 1, 2020

@iocanel note that the selector is different for Deployment and DeploymentConfig. If we were to go with one Decorator, we would need to do type checks and casts to get the desired functionality. Are you sure that's what we want?

@iocanel
Copy link
Contributor

iocanel commented Sep 1, 2020

Need to check.

@iocanel
Copy link
Contributor

iocanel commented Sep 1, 2020

You are right!

Deployment and DeploymentConfig resources are created with their labels set and they are not decorated.
This means that our AddLabelDecorator and RemoveLabelDecorator have logical errors and may cause issues. Also means that its likely to have mismatches when using existing resources.

Let's keep that for now and I'll raise an issue on decorate side.

@geoand
Copy link
Contributor Author

geoand commented Sep 1, 2020

Thanks for checking!

So should we put this on hold and wait for the dekorate fix or merge it as is?

@iocanel iocanel mentioned this pull request Sep 1, 2020
Closed
@iocanel
Copy link
Contributor

iocanel commented Sep 1, 2020

Thanks for checking!

So should we put this on hold and wait for the dekorate fix or merge it as is?

Depends on how fast we need this to get in.

@geoand
Copy link
Contributor Author

geoand commented Sep 1, 2020

It's not super important, just a nice to have

@iocanel
Copy link
Contributor

iocanel commented Sep 1, 2020

It's not super important, just a nice to have

Then let's fix on decorate and align.

@geoand geoand added the triage/on-ice Frozen until external concerns are resolved label Sep 1, 2020
@gsmet
Copy link
Member

gsmet commented Sep 2, 2020

Closing this one then.

@gsmet gsmet closed this Sep 2, 2020
@gsmet gsmet added the triage/invalid This doesn't seem right label Sep 2, 2020
@geoand
Copy link
Contributor Author

geoand commented Sep 2, 2020

We'll it's not invalid, it just needs to be updated when we have a new Dekorate release :).

So I'll reopen so we can address it for 1.8

@geoand geoand reopened this Sep 2, 2020
@gsmet gsmet removed the triage/invalid This doesn't seem right label Sep 2, 2020
iocanel
iocanel reviewed Sep 17, 2020
View reviewed changes
Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

Once #12160 get's in we should be able to proceed with this one.
The latest dekorate version introduces decorators for adding/removing labels from selectors matchingLabes.

@geoand
Copy link
Contributor Author

geoand commented Sep 17, 2020 •
edited
Loading

@iocanel do you want to take over this PR now that dekorate has been updated?

You can go ahead and push into it

iocanel
iocanel reviewed Sep 21, 2020
View reviewed changes
Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

Will do!

@geoand
Copy link
Contributor Author

geoand commented Sep 21, 2020

Thanks!

@iocanel iocanel force-pushed the #11070 branch 2 times, most recently from a21acc1 to c32da06 Compare September 23, 2020 05:31
@geoand geoand removed the triage/on-ice Frozen until external concerns are resolved label Sep 23, 2020
@geoand geoand requested a review from iocanel September 23, 2020 05:44
@geoand geoand added this to the 1.9.0 - master milestone Sep 23, 2020
iocanel
iocanel approved these changes Sep 23, 2020
View reviewed changes
Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

LGTM

@geoand
Copy link
Contributor Author

geoand commented Sep 23, 2020

Thanks for driving it home @iocanel!

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 23, 2020
@geoand geoand merged commit 49ca5ed into quarkusio:master Sep 23, 2020
@geoand geoand deleted the #11070 branch September 23, 2020 10:44
@geoand geoand mentioned this pull request Sep 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iocanel iocanel iocanel approved these changes

Assignees
No one assigned
Labels
area/kubernetes triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.9.0.CR1
Development

Successfully merging this pull request may close these issues.

Kubernetes extension should not use version in labelselectors
3 participants
@geoand @gsmet @iocanel