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

Add an option to fill default namespace in manifest if no other namespace set #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexsomesan
Copy link
Collaborator

This change introduces a falg to enable filling the default namespace in the manifest when no namespace is specified. By default the flag is off and the tool behaves just like before this change.

The solution is not ideal because there doesn't seem to be a robust way to check if a specific resource type requires a namespace without actually making API calls to a cluster. However, there seem to be on average more resources requiring a namespace then not in a specific setup, so it's easier to add namespace everywhere and then remove it from the resources that don't need it.

Of course, if it turns out there is a way to formally identify namespaced resources offline, this feature should make use of that.

formattable := fixMap(doc)

if stripServerSide {
stripServerSideFields(formattable)
}

if defaultNamespace {
Copy link
Owner

Choose a reason for hiding this comment

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

I think instead of doing this mutation here, we can just add this conditional to stripServerSideFields and just not delete the namespace key. I think doing it this way will add a namespace to resources that are not namespaced?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was my first intent as well, but then I realized that stripServerSideFields is only being called when -s is passed on the CLI.
The two operations felt kind of orthogonal so I went like this. Do you mean I should drop the -n flag and just do this as part of stripServerSideFields?

Copy link
Owner

Choose a reason for hiding this comment

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

Well -n will only do something if -s is set anyway, because the default namespace always gets sent back and won't get stripped.

I think maybe we can just remove the delete namespace line from stripServerSideFields

Base automatically changed from master to main March 17, 2021 22:04
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.

2 participants