Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

fleetctl: inform the user about the '-replace' switch in case the units differ #1560

Merged

Conversation

tixxdz
Copy link
Contributor

@tixxdz tixxdz commented Apr 19, 2016

If the unit in the registry is different from the one that is on disk,
and the user did not provide the '--replace' switch flag, then instead
of just printing a warning, let him know about the 'replace' flag.

Closes: #614

@tixxdz
Copy link
Contributor Author

tixxdz commented Apr 19, 2016

This PR depends on #1509

@tixxdz tixxdz added this to the v0.13.0 milestone Apr 19, 2016
@dongsupark
Copy link
Contributor

Looks good. A simple test also works.

@antrik
Copy link
Contributor

antrik commented Apr 20, 2016

I think it would be better to just merge this into the other PR.

Also, this surely should be covered by some functional test? As you didn't have to change the expected result of any existing tests, I assume there is none yet for this case, i.e. a new one needs to be added...

@tixxdz
Copy link
Contributor Author

tixxdz commented Apr 21, 2016

@antrik, sorry but that's not a good idea, each PR on its own now. Other PRs have already been delayed for several days for such cases or many comments without concluding lgtm or comments who lacked the right context of what the PR does... So I'm not adding more patches on top to complicate things.

@antrik
Copy link
Contributor

antrik commented Apr 21, 2016

@tixxdz let's not overgeneralise. In this case it really doesn't make sense to have it separate, as it just changes the code introduced by that other PR; and the change itself should be non-controversial -- so having it separate is just unnecessary code churn and reviewer burden. Just implement the desirable behaviour right away.

(Note that the other PR has not been reviewed by Jon yet -- so it's not like you would be loosing anything by adding improvements to it...)

@dongsupark
Copy link
Contributor

dongsupark commented Apr 22, 2016

@antrik
When I first saw this PR, I actually thought about adding functional tests for checking the warning message. But I didn't do it, because that looked like just one of minor things or nice-to-have. Moreover, think about what would happen if we change the warning message in fleetctl. We would then have to update the functional test again.

Apart from all of that, if you'd really like to see a new functional test for the warning message, sure, I could write it, as I'm also responsible for the functional test for the replace option.

I don't care, whether this PR should be a separate one or not. That's just a matter of taste. I'd just like to see the entire replace option implementation finally merged.

@tixxdz tixxdz force-pushed the tixxdz/fleetctl-warn-user-about-replace-v1 branch 2 times, most recently from 7a410d8 to d479c66 Compare April 25, 2016 13:48
…ts differ

If the unit in the registry is different from the one that is on disk,
and the user did not provide the '--replace' switch flag, then instead
of just printing a warning, let him know about the 'replace' flag.
@tixxdz tixxdz force-pushed the tixxdz/fleetctl-warn-user-about-replace-v1 branch from d479c66 to 7f0cdbc Compare April 25, 2016 14:11
@tixxdz tixxdz merged commit 752ab10 into coreos:master Apr 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants