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

Implement flux debug kustomization command #5117

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Dec 13, 2024

This PR implements the flux debug ks command as specified in #5109

Followup: #5106
Closes: #5109

Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
@stefanprodan stefanprodan added area/kustomization Kustomization related issues and pull requests area/ux In pursuit of a delightful user experience labels Dec 13, 2024
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
}
testEnv.CreateObjectFile(objectFile, tmpl, t)

cases := []struct {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add 2 more tests: one for both flags set, and one for neither flags set?

Copy link
Contributor

Choose a reason for hiding this comment

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

The failure in LoadVariables(), maybe due to non-optional, non-existing substitute from object can also be a test case to make sure the command fails, instead of skipping such failures.

return err
}

if len(ks.Spec.PostBuild.Substitute) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

PostBuild is a pointer and the absence of any post build configuration results in a panic here.

Copy link
Member Author

@stefanprodan stefanprodan Dec 17, 2024

Choose a reason for hiding this comment

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

Added a check at the top

Copy link
Member

Choose a reason for hiding this comment

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

@darkowlzz great catch!

if err != nil {
return err
}
rootCmd.Print(string(status))
Copy link
Contributor

@darkowlzz darkowlzz Dec 17, 2024

Choose a reason for hiding this comment

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

I scaled KC to 0 and created a Kustomization to see the result.

$ flux -n default debug kustomization podinfo --show-status
observedGeneration: -1

I know what it means, but it feels like being a debug command for troubleshooting, we should be able to do some simple analysis and provide some human readable result to help the user understand what's going on, especially in cases like this.
In the usual case, the presence of conditions may help provide some sense of what could be wrong. But in this case, there's no hint of what it means.

If we go ahead and start providing some summary of the result, I think that would increase the scope of what can be done and make it more useful, but complex. Depending on the state, instead of trying to analyze, we can also provide a link to status docs of the respective resource, which the users can read and make sense of themselves. For example https://fluxcd.io/flux/components/kustomize/kustomizations/#kustomization-status.

For the above scenario, we may need to add a line in the docs about what -1 observed generation means.

Copy link
Member Author

@stefanprodan stefanprodan Dec 17, 2024

Choose a reason for hiding this comment

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

I think we should show something like "Resource not initialised" for -1, when we'll implement the debug report.

For the --show-status output, I'm going to take your suggestion and add a link to the status doc on both commands, as a YAML comment.


if len(ks.Spec.PostBuild.Substitute) > 0 {
for k, v := range ks.Spec.PostBuild.Substitute {
finalVars[k] = strings.ReplaceAll(v, "\n", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

This string replacement of the value without any context is confusing. Maybe a link to https://github.com/fluxcd/pkg/blob/main/kustomize/kustomize_varsub.go or a helper function from there with more context would make it more readable to understand why this is being done here. The value transformation can also be tested in one central location and used everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment added

}
testEnv.CreateObjectFile(objectFile, tmpl, t)

cases := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

The failure in LoadVariables(), maybe due to non-optional, non-existing substitute from object can also be a test case to make sure the command fails, instead of skipping such failures.

Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kustomization Kustomization related issues and pull requests area/ux In pursuit of a delightful user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Troubleshoot helpers for Kustomization reconcile failures
3 participants