-
Notifications
You must be signed in to change notification settings - Fork 39
shipperctl: introduce shipperctl chart render
#374
Conversation
Without this, `make build/shipperctl.darwin-amd64` wouldn't rebuild shipperctl even if files in `cmd/shipperctl/cmd/` changed.
It's useful to know what kind of objects shipper is rendering before they reach the api server. For debugging issues with YAML serializing/deserializing, for instance. For that, you can now run `shipperctl chart render --app foobar` or `shipperctl chart render --relelase foobar-deadbeef-0`, and shipperctl will print a whole bundle of YAML to stdout, with each object separated by a `---`. This is currently only useful for applications that already exist, as you can't manually pass standalone chart spec or values. We'll eventually support that once we figure out a non-awkard interface for doing that. from commit 96980a3 by Julio Greff
1822f96
to
3829e9f
Compare
b173af7
to
5ceac1b
Compare
shipperctl chart render
shipperctl chart render
} | ||
}, | ||
} | ||
) |
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 am not sure, but I wonder if there is a better way to not have these variables global?
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 can see it's consistent with the rest of the commands, but I was a bit confused by this 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.
The initial command was getting an appName, pulled it from the cluster and used the chart from it.
I wanted it to be possible to git a yaml of an application that does not exist in the cluster yet.
So with this command you can either give an app name or an application manifest (yaml file name)
The meaning of the variables is in their description in lines 40-41.
Other Go command line code is specifying the flags in a different location so there is no need for global variables. I opted for consistency, but I tend to agree that these global variables are not the best choice. I can start with these commands and have another PR to refactor the rest of shipperctl commands. WDYT?
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 the explanation, I think it makes sense to keep it like that for now. Would you consider this as a potential refactoring for the future?
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.
Of course! I wanted to do it by now but didn't have the time.
Go's way of doing CLIs is not so gracious, but I have seen some examples of nice ones when I was looking into this (shipperctl decommission
shenanigans)
cmd/shipperctl/cmd/chart/app.go
Outdated
} | ||
var application shipper.Application | ||
if appName != "" { | ||
applicationP, err := shipperClient.ShipperV1alpha1().Applications(namespace).Get(appName, metav1.GetOptions{}) |
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.
what does applicationP
stands for?
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.
pointer to an application object
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 found the name a bit confusing.
Could we do:
application, err = *shipperClient.ShipperV1alpha1().Applications(namespace).Get(appName, metav1.GetOptions{})
or use applicationPointer
?
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.
using *
doesn't work because the return type of .Get()
is (*Application, error)
So I renamed
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.
Oh of course 🤦
e46bba3
to
304abe4
Compare
Continuing @juliogreff 's work on
shipperctl chart render
from pr #215It's useful to know what kind of objects shipper is rendering before
they reach the api server. For debugging issues with YAML
serializing/deserializing, for instance.
For that, you can now run
shipperctl chart render app --filename application.yaml
orshipperctl chart render app --appName foobar
orshipperctl chart render relelase foobar-deadbeef-0
and shipperctl will print a whole bundle of YAML to stdout, with each object separatedby a
---
.