Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement basher upgrade --all #119

Closed
wants to merge 4 commits into from
Closed

implement basher upgrade --all #119

wants to merge 4 commits into from

Conversation

pforret
Copy link
Contributor

@pforret pforret commented Oct 5, 2023

as requested, implemented basher upgrade --all as a piped basher-list | xargs -n1 basher upgrade

@@ -14,6 +14,11 @@ if [ "$1" == "--complete" ]; then
exec basher-list
fi

if [ "$1" == "--all" ]; then
basher-list | xargs -n1 basher upgrade
exit 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be exit, so if any of the basher upgrades fail, the non-zero exit code is perserved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. I've adapted it!

@juanibiapina
Copy link
Member

Good start. We're just missing the tests for this case.

@@ -1 +1,2 @@
cellar/
.idea/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please move this ignore line to your personal gitignore?

@@ -14,6 +14,11 @@ if [ "$1" == "--complete" ]; then
exec basher-list
fi

if [ "$1" == "--all" ]; then
basher-list | xargs -n1 basher upgrade
exit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we have tests we can check the behavior of xargs in practice, but I think in this case it will return 0 if at least one basher upgrade is successful, which means the whole script would return 0 even if some packages failed to upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the manpage:

xargs exits with the following status:

              0      if it succeeds

              123    if any invocation of the command exited with status 1-125

              124    if the command exited with status 255

              125    if the command is killed by a signal

              126    if the command cannot be run

              127    if the command is not found

              1      if some other error occurred.

       Exit codes greater than 128 are used by the shell to  indicate  that  a
       program died due to a fatal signal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't answer the question though. If 0 means every basher upgrade succeeded or at least one.

Copy link
Contributor

@pawamoy pawamoy Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was mostly for reference. But I would interpret it as, only return 0 if all invocations returned 0. Otherwise return 123. Anyway, needs confirmation through tests indeed 🙂

Copy link
Contributor

@pawamoy pawamoy Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ echo 'exit $1' > s
$ for i in {0..10}; do echo $i; done | xargs -n1 bash s
$ echo $?
123
$ for i in 0 0 0; do echo $i; done | xargs -n1 bash s
$ echo $?
0

* using 'find ... | while read ...' instead of 'for package in "${packages[@]}"'
* passes shellcheck without warnings
@pforret pforret closed this by deleting the head repository Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants