add snapshot commands #95
Conversation
| updateChannel.NewUpdateChannelCommand(f)) | ||
| updateChannel.NewUpdateChannelCommand(f), | ||
| snapshot.NewSnapshotCommand(f), | ||
| validateSnapshot.NewValidateSnapshotCommand(f)) |
There was a problem hiding this comment.
no strong opinion but that be split in 2 snapshot subcommands:
rhoc deployment snapshot create
rhoc deployment snapshot validate
| }, | ||
| } | ||
|
|
||
| cmdutil.AddClusterAmount(cmd, &opts.clusterAmount) |
There was a problem hiding this comment.
maybe there should be a way to define for which cluster you want to get the snapshot ? my understading is that you can define a number of cluster for which you want a snapshot but that would be unreliable
There was a problem hiding this comment.
the reasoning was querying for X clusters with status=ready and sorted by age.
Right now I'd rather change the validation mechanism to parse the CSV and query for the same clusters afterwards, as to prevent that from happening.
adding the option to set specific clusters is a valid option and can also be done, but right now I just wanted something more straightforward than figuring out know which are the "important clusters" beforehand.
There was a problem hiding this comment.
that's would be fine, main reason was about the consistency between the save and validate
| }, | ||
| } | ||
|
|
||
| cmdutil.AddClusterAmount(cmd, &opts.clusterAmount) |
There was a problem hiding this comment.
I think using numbers may be unreliable i.e. the list cluster may return a different order between snapshot creation and snapshot analysis.
| if opts.clusterAmount < 1 || opts.clusterAmount > 30 { | ||
| return errors.New("clusterAmount must be between 1 and 30") | ||
| } | ||
| if opts.inputFile == "" { |
There was a problem hiding this comment.
you can mark flags as required instead of having to explicit validate them
| const ( | ||
| CommandName = "snapshot" | ||
| CommandAlias = "ss" | ||
| ) |
There was a problem hiding this comment.
Why declaring a constant for this package that is only used once, we could as well directly do:
cmd := &cobra.Command{
Short: "Takes a snapshot of deployments in oldest and ready clusters.",
Use: "snapshot",
In case there is a valid reason, why make it public when it is never accessed from outside the package.
There was a problem hiding this comment.
I could change it, but that pattern is followed by basically all of the commands.
I don't know what is the reason behind that pattern being used. @lburgazzoli?
If there is no reason behind it, I can change it if we don't mind it being the different one.
There was a problem hiding this comment.
we took this commands originally from rhoas and that was the pattern as I recall it was used also outside the package.
we can do whatever make sense for us.
There was a problem hiding this comment.
The only point I can see having it this way is that you get a good idea about what the package is doing by looking at the top of the file, but even for that purpose there are better ways.
I am just challenging some ideas I know we have been dragging from kas, no strong position whatsoever.
| return cmd | ||
| } | ||
|
|
||
| func run(opts *options) error { |
There was a problem hiding this comment.
Why a pointer here? I think implementing this function by value achieve the same goal
There was a problem hiding this comment.
Haha ... I've noticed from kas they tend to use pointers everywhere. Imho pointers are a tool in a wide toolset and should not be used just because it is available. The default approach should be to use value implementation instead of reference.
I know it is a copy and paste issue and I am bringing this up to discussion because I saw it earlier this week, sorry for using your PR as a platform :-)
| return runComparison(opts.f.IOStreams.Out, opts.inputFile, buffer) | ||
| } | ||
|
|
||
| func runComparison(out io.Writer, inputFile string, newSnapshot io.Reader) error { |
There was a problem hiding this comment.
This is only food for thoughts.
I wonder if reading the files into bytes and using bytes.Compare is a better solution.
You could also read the files content, md5 it and compare the two hashes.
None of these proposals can show the diff of the files, but it is a trade of I guess
There was a problem hiding this comment.
I'm going to change that code considerably, but keeping the diff is a must. The reason is that we want to run that command before and after a deploy as to make sure it didn't break anything, and if it does break, we need to know what it was.
There was a problem hiding this comment.
Then your probably want to keep it as it is right now
There was a problem hiding this comment.
@rinaldodev I converted this to a draft to reflect the fact that there will be some refactoring
No description provided.