Skip to content

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

Merged
enekofb merged 2 commits intomainfrom
issues/3626
Sep 5, 2023
Merged

predictable resolution for gitrepository gvk when v1 and v1beta2 exist#3980
enekofb merged 2 commits intomainfrom
issues/3626

Conversation

@enekofb
Copy link
Copy Markdown
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
Copy Markdown
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 👍

// 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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