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.

Sign up for GitHub

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

Jump to bottom

Deprecate --local flag (HMS-3792) #423

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Deprecate --local flag (HMS-3792) #423

wants to merge 3 commits into from
+89 −103

Conversation

kingsleyzissou
Copy link
Contributor

@kingsleyzissou kingsleyzissou commented May 7, 2024 •
edited
Loading

Pulling container images before building would break in the case of authenticated images on podman machine, since the auth file lives on the host and podman machine and won't know about it.

This commit deprecates the --local flag and puts the requirement on the user to ensure that the container is in local storage before initiating the build.

cgwalters
cgwalters reviewed May 7, 2024
View reviewed changes
Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Overall LGTM just two nits

bib/cmd/bootc-image-builder/main.go Outdated Show resolved Hide resolved
bib/cmd/bootc-image-builder/main.go Outdated Show resolved Hide resolved
mvo5
mvo5 reviewed May 7, 2024
View reviewed changes
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.

Thanks for this! Looks good, one tiny idea/suggestion inline.

test/test_build.py Outdated Show resolved Hide resolved
@kingsleyzissou
Copy link
Contributor Author

Arghhh of course this breaks tls-verify, how do we want to handle that? We aren't resolving containers from remote registries anymore, so maybe we want to disable this too

@kingsleyzissou kingsleyzissou force-pushed the deprecate-local branch 2 times, most recently from 996a9ff to 0f0f46f Compare May 7, 2024 15:59
achilleas-k
achilleas-k previously approved these changes May 7, 2024
View reviewed changes
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

The joy and simplicity of a mostly-red PR :)

mvo5
mvo5 previously approved these changes May 8, 2024
View reviewed changes
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!

@kingsleyzissou
Copy link
Contributor Author

flake8 failed on line length. Think it should be all good now 🤞

mvo5
mvo5 previously approved these changes May 8, 2024
View reviewed changes
mvo5
mvo5 previously approved these changes May 8, 2024
View reviewed changes
@kingsleyzissou
Copy link
Contributor Author

Okay it's finally green now. Removing the flag also broke the cross-arch integration tests :/
Simple PR but had so many knock-on effects

mvo5
mvo5 previously approved these changes May 13, 2024
View reviewed changes
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

mvo5
mvo5 reviewed May 13, 2024
View reviewed changes
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.

One more note/question but could be followup (just defensive)

test/test_build.py Outdated Show resolved Hide resolved
test/test_build.py Outdated Show resolved Hide resolved
ondrejbudai
ondrejbudai requested changes May 15, 2024
View reviewed changes
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

I love this! :)

Marking this as request changes, because I want to first talk to the podman desktop folks if this is alright for them, and I want to figure out if we can somehow have bib releases, so users know what version they run, and thus which flags are supported and what's the default behaviour (git SHAs aren't very friendly in this case unfortunately).

@kingsleyzissou kingsleyzissou force-pushed the deprecate-local branch 3 times, most recently from acc9c79 to e3d37c7 Compare May 15, 2024 23:17
@kingsleyzissou kingsleyzissou force-pushed the deprecate-local branch 3 times, most recently from 1c758d1 to 1128e77 Compare September 3, 2024 10:17
@kingsleyzissou kingsleyzissou force-pushed the deprecate-local branch 2 times, most recently from 28ac0e6 to 4f1ac74 Compare September 11, 2024 09:01
mvo5
mvo5 reviewed Sep 11, 2024
View reviewed changes
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.

Thanks, this looks nice! A quick first round of reviews, if this is ready now I will do a deeper dive later today too

bib/cmd/bootc-image-builder/main.go Outdated Show resolved Hide resolved
bib/cmd/bootc-image-builder/main.go Outdated Show resolved Hide resolved
test/test_manifest.py Show resolved Hide resolved
mvo5
mvo5 reviewed Sep 11, 2024
View reviewed changes
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.

Thanks, this looks good, small suggestions inline still more nit-picky, happy to approve.

bib/cmd/bootc-image-builder/main.go Show resolved Hide resolved
test/test_manifest.py Show resolved Hide resolved
test/testutil.py Outdated Show resolved Hide resolved
@kingsleyzissou
Copy link
Contributor Author

Thanks, this looks good, small suggestions inline still more nit-picky, happy to approve.

Thanks! I think the changes are good to do here, especially hiding the flags. So I'll do that and ping you for a review

@kingsleyzissou kingsleyzissou force-pushed the deprecate-local branch 2 times, most recently from a0f0aa1 to 22daa99 Compare September 11, 2024 15:42
mvo5
mvo5 reviewed Sep 11, 2024
View reviewed changes
test/testutil.py Outdated Show resolved Hide resolved
mvo5
mvo5 previously approved these changes Sep 11, 2024
View reviewed changes
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

achilleas-k
achilleas-k previously approved these changes Sep 16, 2024
View reviewed changes
ondrejbudai
ondrejbudai previously approved these changes Sep 17, 2024
View reviewed changes
@ondrejbudai
Copy link
Member

Thanks! 👍

@achilleas-k achilleas-k added this pull request to the merge queue Sep 17, 2024
@achilleas-k
Copy link
Member

achilleas-k commented Sep 17, 2024 •
edited
Loading

Merge conflicts in the queue

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Sep 17, 2024
kingsleyzissou and others added 3 commits October 7, 2024 14:40
@kingsleyzissou @ondrejbudai
bib: deprecate the --local flag
24ebcc6 
Pulling container images before building would break in the case of
authenticated images on podman machine, since the auth file lives on the
host and not podman machine and won't know about it.

This commit deprecates the `--local` flag and warns users when it is
passed to the CLI so that this won't break things for anyone who might
already be using the flag. This change means that the user will have to
ensure that the container is pulled to the local container store before
initiating the build.

Co-authored-by: Ondřej Budai <[email protected]>
@kingsleyzissou
tests: pull containers into local storage
bd2af91 
Ensure that the containers have been copied into local
storage for all test cases.

We need to explicitly pull the container into local containers
storage with the correct arch otherwise cross-arch building
fails. The helper function uses the host-arch as a fallback
when no target arch is provided.
@kingsleyzissou
bib: disable --tls-verify flag
6a95c3e 
Since all containers are coming from local storage and require the user
to pull in the container before-hand, we can disable the `--tls-verify`
flag. The containers will not be resolved from a remote registry but
rather from the local container store.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhatdan rhatdan rhatdan left review comments

@ondrejbudai ondrejbudai ondrejbudai left review comments

@mvo5 mvo5 mvo5 left review comments

@achilleas-k achilleas-k achilleas-k left review comments

@cgwalters cgwalters Awaiting requested review from cgwalters

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kingsleyzissou @achilleas-k @ondrejbudai @cgwalters @mvo5 @rhatdan