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

Two refactoring patches, validation of entrypoint state #89

Merged
merged 7 commits into from
Jan 10, 2024

Conversation

cgwalters
Copy link
Contributor

@cgwalters cgwalters commented Jan 5, 2024

Move build logic into build sub-command verb

In the future it will very likely be useful for us to have
other verbs. For now, this is effectively an implementation
detail though because the entrypoint is changed to always pass build.

(Actually right now this fixes the problem that the completion
verb was being masked)


Move prepare.sh into Go code

It's just ugly to have bits of this in shell script in this
way; it's not complex code and doing it in Go helps keep
the development environment simpler.


Move some helpers into modules

For functional clarity.


Validate that we're in rootful podman

As this is a footgun that multiple people have run into.


Validate that we're running privileged

On general principle.


@cgwalters
Copy link
Contributor Author

What I actually was going to fix is detecting rootless=1 podman containers but I didn't want do that in shell script, so this is prep for having environment mutation/detection in Go, like how we do all similar stuff in bootc in Rust.

@cgwalters
Copy link
Contributor Author

Only lightly tested locally so far...doing some more

@cgwalters cgwalters changed the title Two refactoring patches Two refactoring patches, validation of entrypoint state Jan 5, 2024
@cgwalters
Copy link
Contributor Author

OK now with some logic to validate that we're not running rootless, etc.

@achilleas-k achilleas-k self-requested a review January 6, 2024 13:49
mvo5
mvo5 previously approved these changes Jan 8, 2024
Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you, this is super nice! I have some suggestions/nitpicks inline but no blockers. Feel free to ignore but maybe something in there is useful.

bib/internal/podmanutils/podmanutils.go Outdated Show resolved Hide resolved
bib/internal/podmanutils/podmanutils.go Outdated Show resolved Hide resolved
bib/internal/setup/setup.go Outdated Show resolved Hide resolved
bib/internal/setup/setup.go Outdated Show resolved Hide resolved
bib/internal/setup/setup.go Show resolved Hide resolved
bib/internal/utils/utils.go Outdated Show resolved Hide resolved
@cgwalters
Copy link
Contributor Author

OK resolved all the nits except one

bib/internal/utils/utils.go Outdated Show resolved Hide resolved
mvo5
mvo5 previously approved these changes Jan 10, 2024
Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

This is great, just one tiny comment/nitpick.

bib/internal/setup/setup.go Outdated Show resolved Hide resolved
In the future it will very likely be useful for us to have
other verbs.  For now, this is effectively an implementation
detail though because the entrypoint is changed to always pass `build`.

(Actually right now this fixes the problem that the `completion`
 verb was being masked)

Signed-off-by: Colin Walters <walters@verbum.org>
It's just ugly to have bits of this in shell script in this
way; it's not complex code and doing it in Go helps keep
the development environment simpler.

Signed-off-by: Colin Walters <walters@verbum.org>
For functional clarity.

Signed-off-by: Colin Walters <walters@verbum.org>
As this is a footgun that multiple people have run into.

Signed-off-by: Colin Walters <walters@verbum.org>
In most cases e.g. we do want to show stdout/stderr, and
it's handy to have a debug log when we're running
a subprocess.

While we're here, switch to just forking `cp` in
the setup code.

Signed-off-by: Colin Walters <walters@verbum.org>
Signed-off-by: Colin Walters <walters@verbum.org>
@achilleas-k
Copy link
Member

Clicked the rebase button to get the apt update fix.

@cgwalters
Copy link
Contributor Author

@achilleas-k I think to merge you need to change your review state to approval

@cgwalters cgwalters added this pull request to the merge queue Jan 10, 2024
Merged via the queue into osbuild:main with commit 267738d Jan 10, 2024
10 checks passed
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