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

Refactor ubuntu setup #432

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

Refactor ubuntu setup #432

wants to merge 8 commits into from

Conversation

cpuguy83
Copy link
Member

Extracts all the jammy logic into reusable components.
See individual commits for more details.

@cpuguy83 cpuguy83 requested a review from a team as a code owner November 13, 2024 00:54
@cpuguy83 cpuguy83 added this to the v0.11.0 milestone Nov 13, 2024
@cpuguy83 cpuguy83 force-pushed the refactor_ubuntu_setup branch 2 times, most recently from 7e8bfd1 to 15a8906 Compare November 13, 2024 00:59
pmengelbert
pmengelbert reviewed Nov 13, 2024
View reviewed changes
frontend/deb/distro/install.go Outdated
apt autoclean -y

apt update
apt install -y $@
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
apt install -y $@
apt install -y "$@"

Copy link
Member Author

Choose a reason for hiding this comment

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

Quoting here would turn a list of packages into one single argument to apt which I don't think we want.

Copy link
Contributor

@pmengelbert pmengelbert Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

$@ has special behavior when double-quoted, see the manual

From the manual:

That is, "$@" is equivalent to "$1" "$2" [... and so on]

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL

pmengelbert
pmengelbert requested changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@pmengelbert pmengelbert left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good work overall. But I really think we need to find a solution for installing packages without introducing implicit parameter ordering.

@cpuguy83
Copy link
Member Author

All updated.

@pmengelbert
Copy link
Contributor

Needs a rebase and it's failing a test, but I'll approve once that's sorted.

@cpuguy83
Copy link
Member Author

This is rebased.
The failure was a network flake.

adamperlin
adamperlin requested changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@adamperlin adamperlin left a comment

Choose a reason for hiding this comment

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

One typo that needs to be fixed in the install-with-constraints script, and a few other small comments.

@cpuguy83
Copy link
Member Author

Rebased and comments addressed.

@cpuguy83
Use single common implementation for apt/deb package installs
ef34244 
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
azlinux: remove unused method from "worker" interface
82a169a 
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
Use common implementation for getting base image
04c81b4 
This does actually end up changing the ordering, which I think is more
correct.
If a client has supplied a context with an image name then we should
assume we need to install the distro's base build packages into it,
unlike when the worker context ref is used where we expect that the
provided image is the actual worker.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
jammy: remove special handling of test repos
324f39a 
This was always a bit janky.
We really can't guarentee that all implementations package repos from a
worker to install images into a target, and this work-around was just to
make the test pass.
Instead the test is now just deleted since it doesn't make sense.

While it is technically possible to do this on azlinux people should be
using the custom repo support add extra repos.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
Extract out some common things for building debs
fdd7840 
This allows us to have a common implementation of a bunch of the things
that underpins building a deb package.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
jammy: Make distro config a build handler
972ac4b 
This moves the rest of the logic from the jammy package into the distro
config.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
apt: fix positional handling of constraints options
8f20a8d 
Do not require correct ordering of constraints options for apt/deb
functions.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
Minor fixups in ubuntu distro setup
d1fec18 
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
Copy link
Member Author

Rebased again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmengelbert pmengelbert pmengelbert requested changes

@adamperlin adamperlin adamperlin requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.11.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cpuguy83 @pmengelbert @adamperlin