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

fix(kubernetes): do not fail to deploy List kinds (e.g. ConfigMapList) #4501

Merged
merged 2 commits into from Jun 1, 2023
Merged

fix(kubernetes): do not fail to deploy List kinds (e.g. ConfigMapList) #4501

merged 2 commits into from Jun 1, 2023

Conversation

stefreak
Copy link
Member

@stefreak stefreak commented May 31, 2023 •
edited

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #4500

Special notes for your reviewer:

@stefreak stefreak requested a review from a team May 31, 2023 15:22
@stefreak
fix(kubernetes): do not fail to deploy List kinds (e.g. configMapList)
5374942 
Fixes #4500

Co-authored-by: Srihas Konduru <srihas-g@users.noreply.github.com>
@stefreak stefreak changed the title fix(kubernetes): do not fail to deploy List kinds (e.g. configMapList) fix(kubernetes): do not fail to deploy List kinds (e.g. ConfigMapList) May 31, 2023
Orzelius
Orzelius previously approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@Orzelius Orzelius left a comment

Choose a reason for hiding this comment

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

Thanks for figuring this out!

Walther
Walther previously approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@Walther Walther left a comment

Choose a reason for hiding this comment

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

🎉

@stefreak
Copy link
Member Author

@Orzelius @Walther @edvald I wonder if this would break if there was a custom resource definition with custom resources ending with *List. This code would fail then. Maybe it would be better to hardcode a list of specific kinds of resources instead of just looking for the *List ending. What do you think?

@stefreak
Copy link
Member Author

Maybe it would be safe enough to only flatMap the List if it actually has items– and otherwise keep it as-is

@edvald
Copy link
Collaborator

edvald commented May 31, 2023

I think we should only do this for resources with apiVersion: v1, and make sure we gracefully fail on invalid resources. If we do that, it should be all good.

@stefreak
Copy link
Member Author

Kustomize also treats anything with *List as a List, and errors if items is not a list. So I guess this is ok for now.

@stefreak
improvement: slightly increase defensiveness
0e46a7e 
Maybe the List is actually a CRD? Seems to be unlikely as other tools like Kustomize also treat kinds matching `*List` as lists.
But add a better error message here if items contains something unexpected just in case.

This logic is more or less equivalent to https://github.com/kubernetes-sigs/kustomize/blob/cf3e81b590ab1fd7fc8f50849535f44bfea0d355/api/resource/factory.go#L550
@stefreak stefreak dismissed stale reviews from Walther and Orzelius via 0e46a7e May 31, 2023 17:32
@stefreak stefreak requested review from Orzelius and Walther May 31, 2023 17:33
@stefreak
Copy link
Member Author

stefreak commented May 31, 2023 •
edited

@edvald I now implemented a logic equivalent to what seems to work for Kustomize – looking ok for you?

@stefreak stefreak merged commit 25e1637 into main Jun 1, 2023
36 of 38 checks passed
@stefreak stefreak deleted the fix/4500 branch June 1, 2023 09:57
@edvald
Copy link
Collaborator

edvald commented Jun 1, 2023

I still think we should only do this for core API resources, basically just apiVersion: v1. Otherwise this may break some CRDs that can be named "SomethingList" and have completely unrelated semantics.

@vvagaytsev vvagaytsev mentioned this pull request Jul 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Orzelius Orzelius Orzelius approved these changes

@Walther Walther Awaiting requested review from Walther

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

0.13: [Bug]: kind=Deploy type=kubernetes fails with kind: ConfigMapList
4 participants
@stefreak @edvald @Walther @Orzelius