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

Fix podman_tooling script #136

Merged
merged 2 commits into from
Jun 14, 2022
Merged

Conversation

cevich
Copy link
Member

@cevich cevich commented May 31, 2022

Depends on: #134

This script is needed by both podman and buildah CI. However a bug in
the line export GOPATH="${GOPATH:/var/tmp/go}" was causing it to
silently exit before doing it's job. Fortunately the downstream build
systems are somewhat tolerant, so the problem wasn't noticed at image
build time.

Simplify the script by requiring it to be called under $SUDO, this
allows moving a bunch of golang variable definitions into the script and
also making it set -e so failures are noticed sooner.

Ref: Silent failure example: https://cirrus-ci.com/task/6089758945312768?logs=main#L1905-L1906

@cevich cevich mentioned this pull request May 31, 2022
Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

LGTM

@cevich
Copy link
Member Author

cevich commented May 31, 2022

force-push: rebased on @lsm5 #134 to make sure my podman_tooling.sh changes work.

@lsm5
Copy link
Member

lsm5 commented May 31, 2022

lilto command not found

Copy link
Collaborator

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

All the sudo bash makes me cringe. Why not just add a hashbang-bash line to the script, make it 755, and invoke it directly? (This is a pet peeve of mine: let the script itself determine the interpreter it wants).

Also, CI is failing with a "kubik" error; sounds like the Ubuntu issue from a week or two ago.

cache_images/fedora_packaging.sh Outdated Show resolved Hide resolved
cache_images/podman_tooling.sh Outdated Show resolved Hide resolved
cache_images/podman_tooling.sh Outdated Show resolved Hide resolved
cache_images/podman_tooling.sh Outdated Show resolved Hide resolved
Co-authored-by: Chris Evich <cevich@redhat.com>
Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>
@cevich
Copy link
Member Author

cevich commented Jun 1, 2022

All the sudo bash makes me cringe. Why not just add a hashbang-bash line to the script, make it 755, and invoke it directly?

I was afraid of making it appear like users should ever try and execute that script. I s'pose I could make it executable in the setup script.

Also, CI is failing with a "kubik" error; sounds like the Ubuntu issue from a week or two ago.

This is expected since this PR depends on #134

@cevich cevich force-pushed the fix_podman_tooling branch 2 times, most recently from 01bb5c3 to c1258d4 Compare June 1, 2022 16:10
@cevich
Copy link
Member Author

cevich commented Jun 1, 2022

In fact, let's try it with #134 now and see if it builds successfully.

This script is needed by both podman and buildah CI.  However a bug in
the line `export GOPATH="${GOPATH:/var/tmp/go}"` was causing it to
silently exit before doing it's job.  Fortunately the downstream build
systems are somewhat tolerant, so the problem wasn't noticed at image
build time.

Simplify the script by requiring it to be called under $SUDO, this
allows moving a bunch of golang variable definitions into the script and
also making it `set -e` so failures are noticed sooner.

Signed-off-by: Chris Evich <cevich@redhat.com>
@cevich
Copy link
Member Author

cevich commented Jun 1, 2022

Force-push: Woopsie, forgot to do the chmod +x to the script.

@github-actions
Copy link

github-actions bot commented Jun 1, 2022

Cirrus CI build successful. Image ID c6193881921355776 ready for use.

@edsantiago
Copy link
Collaborator

6b66a74 LGTM (I did not review 0220c22)

@cevich
Copy link
Member Author

cevich commented Jun 1, 2022

Thanks @edsantiago

@cevich
Copy link
Member Author

cevich commented Jun 3, 2022

Status note: Waiting for #134 to merge, then this can go in.

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.

None yet

3 participants