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

predictable resolution for gitrepository gvk when v1 and v1beta2 exist #3980

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

enekofb
Copy link
Contributor

@enekofb enekofb commented Sep 1, 2023

Closes #3626

What changed?

Adjusted weights to avoid the ranking function computes v1 and v1beta2 with the same value.

Why was this change made?

We noticed that sometimes gvk resolution for gitrepositories was returning v1beta2 instead of v1 which was expected. The reason was that both v1 and v1beta2 level had the same ranking value.

Therefore v1 or v1beta2 was returned depending on which one was first introduced in primaryKinds. Given AllKnownTypes was randomly returning either of them, we got to that unpredictable behavior.

How was this change implemented?

Adjusted weights to avoid the ranking function computes v1 and v1beta2 with the same value.

How did you validate the change?

Added test case for supporting the specific case and for the gitRepository from primaryKinds.

Also tested e2e

Screenshot 2023-09-01 at 12 00 11

@enekofb enekofb force-pushed the issues/3626 branch 3 times, most recently from 31abffa to c961c96 Compare September 1, 2023 10:26
@enekofb enekofb requested a review from chanwit September 1, 2023 10:26
@enekofb enekofb changed the title predictable resolution for gitrepository gvk predictable resolution for gitrepository gvk when v1 and v1beta2 exists Sep 1, 2023
@enekofb enekofb marked this pull request as ready for review September 1, 2023 10:37
@enekofb enekofb changed the title predictable resolution for gitrepository gvk when v1 and v1beta2 exists predictable resolution for gitrepository gvk when v1 and v1beta2 exist Sep 1, 2023
@enekofb enekofb requested a review from jpellizzari September 1, 2023 11:03
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

A bit confused on the logic in primarykinds.go, but that is not the concern of the PR, so 👍

@@ -25,7 +25,7 @@ const (
// It's multiplied with the digit following the "beta" suffix.
// Beta versions are more stable than alpha versions but might still contain bugs.
// They are given a higher weight than alpha versions.
betaWeight = 500
betaWeight = 50
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for clarification: we determine which kinds to use at runtime?

Copy link
Contributor Author

@enekofb enekofb Sep 5, 2023

Choose a reason for hiding this comment

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

yes they are resolved at startup here and here2

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a bit of an anti-pattern...

@enekofb enekofb enabled auto-merge (squash) September 5, 2023 10:41
@enekofb enekofb merged commit 5a97fbb into main Sep 5, 2023
14 checks passed
@enekofb enekofb deleted the issues/3626 branch September 5, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The YAML tab displays a wrong version of CRD
3 participants