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

Use resource prefix when apiVersion is v1 #364

Closed

Conversation

gravesm
Copy link
Member

@gravesm gravesm commented Feb 3, 2022

SUMMARY

When getting a resource from the core api group, the prefix was not
passed, leading the lookup to happen in all api groups. This broad
search is not really necessary and leads to problems in some corner
cases, for example, when an api is deleted after the api group list is
cached.

This fix uses the 'api' prefix when the apiVersion is 'v1', as this is
almost certainly what the user wants. As a fallback, to retain backwards
compatibility, the old behavior is used if the first lookup failed to
find a resource. Given that the module defaults to 'v1' for the
apiVersion, there are likely many cases where a resource, such as
StatefulSet, is used while failing to provide an apiVersion. While
technically incorrect, this has worked in most cases, so we probably
shouldn't break this behavior.

Fixes #351
Depends-On: #366

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

When getting a resource from the core api group, the prefix was not
passed, leading the lookup to happen in all api groups. This broad
search is not really necessary and leads to problems in some corner
cases, for example, when an api is deleted after the api group list is
cached.

This fix uses the 'api' prefix when the apiVersion is 'v1', as this is
almost certainly what the user wants. As a fallback, to retain backwards
compatibility, the old behavior is used if the first lookup failed to
find a resource. Given that the module defaults to 'v1' for the
apiVersion, there are likely many cases where a resource, such as
StatefulSet, is used while failing to provide an apiVersion. While
technically incorrect, this has worked in most cases, so we probably
shouldn't break this behavior.
Copy link
Contributor

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

/lgtm

nice solution

@softwarefactory-project-zuul
Copy link

Build failed.

@gravesm
Copy link
Member Author

gravesm commented Feb 3, 2022

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@Akasurde
Copy link
Member

Akasurde commented Feb 4, 2022

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@Akasurde Akasurde force-pushed the fix-resource-request branch from f48f2f5 to 8eeddf2 Compare February 4, 2022 11:54
@softwarefactory-project-zuul
Copy link

Build failed.

@gravesm gravesm closed this Feb 4, 2022
@gravesm gravesm reopened this Feb 4, 2022
@goneri
Copy link
Member

goneri commented Feb 4, 2022

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@goneri
Copy link
Member

goneri commented Feb 4, 2022

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@Akasurde
Copy link
Member

Akasurde commented Feb 7, 2022

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@Akasurde
Copy link
Member

Akasurde commented Feb 8, 2022

recheck

@softwarefactory-project-zuul
Copy link

Build failed.

@Akasurde Akasurde closed this Feb 8, 2022
@Akasurde Akasurde reopened this Feb 8, 2022
@softwarefactory-project-zuul
Copy link

Build failed.

@Akasurde Akasurde closed this Feb 8, 2022
@Akasurde Akasurde reopened this Feb 8, 2022
@softwarefactory-project-zuul
Copy link

Build failed.

@Akasurde
Copy link
Member

Akasurde commented Feb 8, 2022

superseded by #371

@Akasurde Akasurde closed this Feb 8, 2022
@Akasurde
Copy link
Member

Akasurde commented Feb 8, 2022

Due to CI migration, opening a new PR #371

@gravesm gravesm deleted the fix-resource-request branch May 12, 2022 18:42
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.

Failed to find exact match for v1.Namespace by [kind, name, singularName, shortNames]
4 participants