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

profile.d: don't try to source non-existent os-release file #1475

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ptalbert
Copy link

@ptalbert ptalbert commented Apr 2, 2024

When /etc/os-release does not exist we directly try to source /usr/lib/os-release. This causes a 'No such file or directory' error when entering a toolbox which has neither.

Fix this by checking for the presence of /usr/lib/os-release before sourcing it, just like what is done for /etc/os-release.

When /etc/os-release does not exist we directly try to source
/usr/lib/os-release. This causes a 'No such file or directory'
error when entering a toolbox which has neither.

Fix this by checking for the presence of /usr/lib/os-release before
sourcing it, just like what is done for /etc/os-release.

Signed-off-by: Patrick Talbert <[email protected]>
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/5663ec31cedd4dd496124fcd2eac3897

✔️ unit-test SUCCESS in 4m 57s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 38s
✔️ unit-test-restricted SUCCESS in 4m 01s
system-test-fedora-rawhide TIMED_OUT in 1h 20m 30s
✔️ system-test-fedora-39 SUCCESS in 35m 17s
✔️ system-test-fedora-38 SUCCESS in 35m 21s

@ptalbert ptalbert marked this pull request as ready for review April 2, 2024 13:06
@mh21
Copy link

mh21 commented Apr 2, 2024

@ptalbert should it set some defaults for the expected variables that now are unset?

@ptalbert
Copy link
Author

ptalbert commented Apr 2, 2024 via email

Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

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

Welcome to Toolbx! :)

It's true that the complete absence of os-release(5) makes no difference to the profile.d/toolbox.sh snippet as it's today. However, the toolbox(1) binary does make extensive use of os-release(5) for some critical logic.

The toolbox(1) binary currently uses the github.com/acobaugh/osrelease Go module to parse os-release(5) and it has the same structure as the profile.d/toolbox.sh snippet. ie., /etc/os-release may be missing, but it somewhat assumes that /usr/lib/os-release is present. eg., there's no fallback to linux if ID is absent.

Therefore, if we are going to make changes to support set-ups without any os-release(5), we need to ensure that the whole thing, including the binary and the snippet, works. This brings me to the question: how did you end up without any os-release(5)?

@mh21
Copy link

mh21 commented Apr 9, 2024

This brings me to the question: how did you end up without any os-release(5)?

By using a RHEL6-based container image 😂. But if this is really going to ripple through multiple pieces of code here, maybe it would be easier to just fake an os-release file in the container image -> https://gitlab.com/cki-project/containers/-/merge_requests/539

@ptalbert
Copy link
Author

@debarshiray like Michael said, I noticed this when using a rhel6 container image with toolbox. Every time you enter the toolbox (or source your profile in the container) there is this warning about a missing /usr/lib/os-release.

For our purposes I am happy to take Michael's solution but maybe there are a few unrelated people out there using toolbox with containers that predate os-release 🤷. If there is something else I can test please let me know.

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.

3 participants