feat: add check-update subcommand#2463
feat: add check-update subcommand#2463natescherer wants to merge 36 commits intocopier-org:masterfrom
check-update subcommand#2463Conversation
sisp
left a comment
There was a problem hiding this comment.
Thanks for contributing this feature to Copier, @natescherer! 🙇
I've left a couple of inline comments.
|
@sisp While I'm working on those changes, would appreciate your thoughts on the two questions I raised at the bottom of the PR, thanks. 🙏 |
sisp
left a comment
There was a problem hiding this comment.
Regarding the exit code: I think exiting with 0 when there's no update (i.e., the project is up-to-date) and with non-zero otherwise seems reasonable. At least Cruft follows a similar approach. The specific non-zero exit code is another question. Cruft exits with 1 when there is an update, but 1 is already used for, e.g., precondition errors like a missing old template reference. Exit code 2 is used for indicating unsafe templates without explicit user trust, but this error can only occur for copy and update operations but never when checking for an update. I think it's fine to use the same exit code for different errors by different subcommands.
[...] but if I was designing this from scratch, I wouldn't consider an update being available an error in the classic sense
What exactly do you mean by "in the classic sense"?
Regarding documentation: How about adding a section on our "Updating a project" page, similar to Cruft's corresponding section?
|
Sounds good re: the documentation. As far as what I mean by "in the classic sense" this is probably my SRE brain talking, but I tend to see any application ending with a non-zero status code as indicating that there is some failure in execution, but there's no failure/error/issue when the sub command checks and sees there is an update; the update checking process worked just fine. Hopefully that makes sense? |
Co-authored-by: Sigurd Spieckermann <2206639+sisp@users.noreply.github.com>
|
Hi @sisp, I've completed updating the following:
Let me know if there's anything else you'd like me to change! |
|
Okay, should now be up-to-date with the latest round of revisions. I've also resolved the conflict with
Let me know if anything else needs changed! |
|
Fixed a couple other issues I noticed after syncing with |
sisp
left a comment
There was a problem hiding this comment.
Looks pretty good already, I've left a few more comments. 🫣
Co-authored-by: Sigurd Spieckermann <2206639+sisp@users.noreply.github.com>
|
Latest round of changes completed. Let me know if there's anything else you'd like me to change! |
sisp
left a comment
There was a problem hiding this comment.
Nice! 🤩 Just some minor polishing, then we should be good to go. 🚀
Co-authored-by: Sigurd Spieckermann <2206639+sisp@users.noreply.github.com>
Co-authored-by: Sigurd Spieckermann <2206639+sisp@users.noreply.github.com>
Co-authored-by: Sigurd Spieckermann <2206639+sisp@users.noreply.github.com>
|
Ready for your review again! |
Hello! This is my attempt to complete the work that was done in #1031. (Should close #1020, as well.)
What's Changed
Here's what I've changed versus the previous PR, by type:
Outstanding changes requested in discussions on old PR
check-update52was discussed in the previous PR but I changed it based on Ambiguous exit code 2 #1328Necessary Updates
Other Changes
--check-update-output-as-jsonflag for integrating with CI/scripts/etcQuestions/To-Do
I have a few questions for the maintainers:
--check-update-output-as-jsonprovides a hook for scripts to detect if there is an update, from the discussions I saw before that seemed to be the overall intention of the exit codedocs/checking_for_updates.md?Thanks for making such an awesome tool! Please let me know if there are changes you'd like me to make.