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.

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

SECURESIGN-1015 | Make sure route selector labels and managed annos are recconciled #613

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

JasonPowr
Copy link
Contributor

No description provided.

@osmman
Copy link
Contributor

osmman commented Sep 12, 2024

@bouskaJ Could you please review this PR? You have a deeper understanding of base_action

@bouskaJ
Copy link
Collaborator

bouskaJ commented Sep 13, 2024

Do we need to do this by operator? Resource labels are not updated by Ensure function at all so user can freely modify them. This is just first question that popped up in my head. Maybe I am missing something.

@JasonPowr
Copy link
Contributor Author

Do we need to do this by operator? Resource labels are not updated by Ensure function at all so user can freely modify them. This is just first question that popped up in my head. Maybe I am missing something.

@osmman @bouskaJ, You make a good point, I just tested this to 100% confirm and adding the labels manually works without issue. I guess the only thing we loose here by not doing it is the convince of being able to configure it through the API. I suppose it may also be confusing for the customer, if they see the option to configure routeSelectorLabels for ingress sharding they might assume they can add it through the api and update exsisting ingress and route objects.

@osmman WDYT?

@JasonPowr JasonPowr force-pushed the make-sure-labels-apply-to-created-ingress branch from 72c3b2f to 457bd3f Compare September 18, 2024 18:33
@JasonPowr JasonPowr changed the title SECURESIGN-1015 | Make sure Route Selector Labels are reconciled SECURESIGN-1015 | Make sure route selector labels and managed annos are recconciled Sep 18, 2024
@JasonPowr
Copy link
Contributor Author

/test tas-operator-e2e

1 similar comment
@JasonPowr
Copy link
Contributor Author

/test tas-operator-e2e

@JasonPowr JasonPowr requested review from osmman and bouskaJ and removed request for osmman and bouskaJ September 19, 2024 07:59
@JasonPowr
Copy link
Contributor Author

@osmman @bouskaJ When you guys have a minute can you take a look? thanks

@JasonPowr JasonPowr force-pushed the make-sure-labels-apply-to-created-ingress branch from 457bd3f to 4a8eb65 Compare September 19, 2024 13:28
@JasonPowr
Copy link
Contributor Author

@bouskaJ @osmman This is good for a re-review, when you guys have a minute, thanks :)

Copy link

openshift-ci bot commented Sep 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JasonPowr, osmman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 780ec7a into main Sep 20, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants