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

handle git pull errors #768

Closed
wants to merge 1 commit into from
Closed

Conversation

ionous
Copy link
Contributor

@ionous ionous commented Jul 6, 2024

re: #705; today i learned:

by default, bash doesn't exit automatically when a command fails ( that way, it can handle a failed result and do interesting things ). set -e however turns on automatic exit, and its at the top of the ./shift script.... but then it gets disabled when any of the sub_* commands are called ( at the bottom of the script )

we actually need the no-auto-exit behavior for the "waitUntilServiceIsReady" command, and no-auto-exit is the normal bash default -- so instead of turning it on for sub_pull()... instead i added || exit.

because success is zero and error codes are non-zero, a normal shortcircuiting "or" would only evaluate the right hand expression on success -- but apparently bash has special handling of exit codes ( or maybe all numbers? ) that reverses the expected behavior. 🎉

[ this is somewhat hard to test because you need to create a pull conflict. one way is to explicitly checkout some earlier commit, then manually overwrite your local shift script with the one from this pr, then run "./shift pull" -- it should see the detached head status and punt without launching docker. ]

@ionous
Copy link
Contributor Author

ionous commented Jul 6, 2024

in this pr, i also removed the "is macOS" use docker compose (with a space) logic. there is no special branching needed. docker-compose (with a dash) works fine on macos

the dash and space versions are different applications. the space version is written in go, the dash version is written in python. go is the current version, python is deprecated. production uses the dash version, and the space version doesn't seem to exist. ( when you type "docker compose --help" on the server you only get general docker help information; you do not get specialized help about the compose command as expected. )

making sure macos and production are actually using the same compose code seems good. so "docker-compose" feels right for all platforms until production can upgrade.

production:

Docker version 24.0.5, build 24.0.5-0ubuntu1~20.04.1

my local:

Docker version 26.1.1, build 4cf5afa

https://docs.docker.com/compose/intro/history/

not sure when, or if we should update the docker environment on production. 24.0.5 is about a year out of date https://docs.docker.com/engine/release-notes/24.0/#2405 -- 2023-07-24

maybe a good separate ticket

@carrythebanner
Copy link
Collaborator

Filed Docker update on prod as #772.

shift Outdated
echo "not sure how to handle your OS' version of docker. One of the above should work!"
exit 255
fi
docker-compose $@
Copy link
Collaborator

Choose a reason for hiding this comment

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

We originally did this docker compose vs docker-compose dance to work around some other failures, but that was in 2021/2022 and Docker v20.10 — see commit notes here:
32d9bd2
7ab6a19

As long as this runs in Docker v24 or later on Ubuntu (production) and macOS (both Apple Silicon and Intel), we're all good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Theoretically it might also run on Windows, but we haven't actively developed/tested that. If it fails on Windows it could be for other reasons and wouldn't be a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: production, because it's 'linux' it currently uses the "docker-compose"; so it won't see any change.
re: windows, because this a bash script, it will only run under "linux on windows" anyway, which it sees as 'linux' so it won't see any change. ( someday: i'd love to convert this to a npm script, so that linux on windows isn't needed. )

all that said, it's definitely bad practice to do two things at once. i was just being lazy. i'll make a separate pr for this, and pull ( oh, no puns ) this out.

@ionous ionous force-pushed the pull-errors-705 branch from 2dcba9e to 087402b Compare July 8, 2024 05:06
@ionous ionous closed this Jul 8, 2024
@ionous
Copy link
Contributor Author

ionous commented Jul 8, 2024

i've closed this, and will reopen one with a fix.
i tried restoring the lines, but git did strange things with the indentation
( the bash file is unfortunately a mix of tabs and spaces )

@ionous ionous deleted the pull-errors-705 branch July 16, 2024 01:48
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.

2 participants