-
Notifications
You must be signed in to change notification settings - Fork 122
feat: add deployment flag for printing out the graph of resources #1064
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
base: develop
Are you sure you want to change the base?
Conversation
f0af80a
to
941b555
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating the PR. I added some questions and suggestions, but can you please provide an example of the output?
return err | ||
} | ||
|
||
if o.DeployFlags.ChangeGraphFile != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason we do this check in this line and in line 528?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having it here makes it clear what the cli logic actually does. The check in the writeChangeGraphToFile
was purely defensive/copying the pattern of writeAppMetadataToFile
. Happy to remove that to remove any confusion
cmd.Flags().BoolVar(&s.DisableGKScoping, "dangerous-disable-gk-scoping", | ||
false, "Disable scoping of resource searching to used GroupKinds") | ||
|
||
cmd.Flags().StringVar(&s.ChangeGraphFile, "change-graph-file-output", "", "Render the deployment graph to a file") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense that this option would only print the graph and not apply it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it as option to print out the graph that will be used to apply. This flag can then be used either with the apply or with the --diff-run
. The hope is to be able to use the information in the graph to display the plan for review ahead of time and use it to show progress of the deployment when it's being applied
pkg/kapp/cmd/app/deploy.go
Outdated
return nil | ||
} | ||
|
||
func (o *DeployOptions) writeChangeGraphToFile(graph *ctldgraph.ChangeGraph) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this function makes more sense to be part of the carvel.dev/kapp/pkg/kapp/diffgraph
package
pkg/kapp/cmd/app/deploy.go
Outdated
cmdtools "carvel.dev/kapp/pkg/kapp/cmd/tools" | ||
ctlconf "carvel.dev/kapp/pkg/kapp/config" | ||
ctldiff "carvel.dev/kapp/pkg/kapp/diff" | ||
"carvel.dev/kapp/pkg/kapp/diffgraph" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this import the same as the next line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDE snafu, will remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example output
{
"nodes": [
{
"id": "v1.6a86088b196cff2407b0eb62e4ea2c30",
"data": {
"name": "localstack-aws-secret",
"namespace": "default",
"changeGroups": [
"change-groups.kapp.k14s.io/pod-related"
],
"groupKind": {
"group": "",
"kind": "Secret"
},
"op": "upsert"
}
},
{
"id": "v1.f3b750c8ef32f6f406ad8f65f507c5bd",
"data": {
"name": "provider-kubernetes-admin-binding",
"namespace": null,
"changeGroups": [
"change-groups.kapp.k14s.io/rbac-role-bindings",
"change-groups.kapp.k14s.io/rbac"
],
"groupKind": {
"group": "rbac.authorization.k8s.io",
"kind": "ClusterRoleBinding"
},
"op": "upsert"
}
},
{
"id": "v1.13f78867be9c1e85b4e0ff14ddde74d5",
"data": {
"name": "xservicecomposition.example.com",
"namespace": null,
"changeGroups": [
"example.com/crossplane-xrds"
],
"groupKind": {
"group": "apiextensions.crossplane.io",
"kind": "CompositeResourceDefinition"
},
"op": "upsert"
}
}
],
"edges": [
{
"source": "v1.6a86088b196cff2407b0eb62e4ea2c30",
"target": "v1.f3b750c8ef32f6f406ad8f65f507c5bd"
}
]
}
cmd.Flags().BoolVar(&s.DisableGKScoping, "dangerous-disable-gk-scoping", | ||
false, "Disable scoping of resource searching to used GroupKinds") | ||
|
||
cmd.Flags().StringVar(&s.ChangeGraphFile, "change-graph-file-output", "", "Render the deployment graph to a file") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it as option to print out the graph that will be used to apply. This flag can then be used either with the apply or with the --diff-run
. The hope is to be able to use the information in the graph to display the plan for review ahead of time and use it to show progress of the deployment when it's being applied
return err | ||
} | ||
|
||
if o.DeployFlags.ChangeGraphFile != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having it here makes it clear what the cli logic actually does. The check in the writeChangeGraphToFile
was purely defensive/copying the pattern of writeAppMetadataToFile
. Happy to remove that to remove any confusion
pkg/kapp/cmd/app/deploy.go
Outdated
cmdtools "carvel.dev/kapp/pkg/kapp/cmd/tools" | ||
ctlconf "carvel.dev/kapp/pkg/kapp/config" | ||
ctldiff "carvel.dev/kapp/pkg/kapp/diff" | ||
"carvel.dev/kapp/pkg/kapp/diffgraph" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDE snafu, will remove
941b555
to
4162098
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not pointing this out earlier, but do you mind adding 1 or 2 tests just to ensure that the functionality is working as expected?
The PR code as a whole looks good
This allows for loading the graph data into visualization tools and to better reason about the ordering of a deployment Signed-off-by: Erik Miller <[email protected]>
@joaopapereira seems like the test that's failing was already failing on mainline |
@erikmiller-gusto yes I am aware, @devanshuVmware is looking into it and when he gets it fixed we can rebase your PR |
@joaopapereira @devanshuVmware any update on that? |
What this PR does / why we need it:
Adds a deployment flag for printing out the graph of resources
This allows for loading the graph data into visualization tools and to better reason about the ordering of a deployment
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: