feat(fleetctl): add template unit support to fleetctl start/submit #480
Conversation
|
Any comments on this one? |
|
@jonboulle What about the case where you have a template unit locally, but you want to start an instance of it without first submitting that template? |
|
fixed |
| if err != nil { | ||
| return fmt.Errorf("failed getting Unit(%s) from file: %v", jobName, err) | ||
| log.V(1).Infof("Error retrieving template Job(%s) from Registry: %v", uni.Template, err) |
There was a problem hiding this comment.
This change doesn't make sense to me. Why are we continuing operation if we encountered an error?
There was a problem hiding this comment.
See lines 397-398 - this is existing behaviour
|
Reee baysed |
| if err != nil { | ||
| return fmt.Errorf("failed getting Unit(%s) from file: %v", jobName, err) | ||
| return fmt.Errorf("Error retrieving template Job(%s) from Registry: %v", uni.Template, err) |
There was a problem hiding this comment.
error strings should be lowercase
|
functionally looks fine, just getting picky about things |
|
The whole point of this (and #520) was to undo that change! 😕 |
|
I'm confused. Can we discuss in person? |
|
Chief Picky Officer B. Waldon of the 1st Fleet Battalion |
|
Reporting for duty. |
|
Once we have a logo I am going to get you a sailor cap |
|
Lowercasefied errors and squashed. |
| return 1 | ||
| } | ||
| } | ||
| if err := lazyCreateJobs(args, sharedFlags.Sign); err != nil { |
There was a problem hiding this comment.
This is actually an important change. The code you're replacing fails if it cannot create a job. Now if someone makes a change to a local unit and calls fleetctl submit X, the command will succeed, and could be misleading.
There was a problem hiding this comment.
You're right, but a) we do warn about this explicitly:
This operation is idempotent; if a named unit already exists in the cluster, it will not be resubmitted.
and b), this is the behaviour today for fleetctl start foo.service && vi foo.service && fleetctl start foo.service; I don't think it's any less misleading there.
So we should take this opportunity to solve for both.
There was a problem hiding this comment.
Previously I'd thought about adding a check - if a unit already exists in the registry and locally, compare hashes and print a warning to the user. Thoughts? (Would do that as a subsequent change, not here)
There was a problem hiding this comment.
Yes, that comparison/warning would be great.
|
lgtm |
|
Merging this and will follow up with warning. |
feat(fleetctl): add template unit support to fleetctl start/submit
Building on #475 and #479, this is a first pass at adapting fleetctl to support creation of instance units based on template units already stored in the Registry