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

build images using new hack/build-image script #135

Merged
merged 4 commits into from
Jul 17, 2023

Conversation

phlogistonjohn
Copy link
Collaborator

@phlogistonjohn phlogistonjohn commented May 31, 2023

Conflicts with #133 #133 is closed.

This PR is a working prototype of an alternate approach to #133. In this patch series I remove a lot of the complexity from the makefile and add a new script build-image written in python to the hack dir.

While the python ends up being longer, it allows the use of real data structures and is (IMO) a lot more straightforward and easy to read than the makefile based changes in #133. The use of python's argparse library means each command line option can be, and is, documented.

I want the team to take a look at the overall diff of both and we can debate which one is the better overall approach... or just take both as ideas and suggest a 3rd approach. ;-)

Due to the recent churn related to the addition of workflow matrix in Github CI and a bunch of other changes getting this back into shape took the better part of a day. However, I think this is now in a usable and reviewable state. The current suite of tests pass.


I've tried to add a few helpful options, including --print and --print-tags which displays the FQINs and FQINs plus additional tags respectively. These additional features made the script even longer but they paid off for me while I was debugging issues with the CI.

Future PRs will need to change how we handle the "special tags" like latest, but first we'll need to move the scripts and such to the FQINs after this lands. Later, I plan on using manifests so that we can build and test {arm,amd}64 images seperately but then push them to qauy under the "latest" tag.

@mergify
Copy link

mergify bot commented May 31, 2023

This pull request now has conflicts with the target branch. Please resolve these conflicts and force push the updated branch.

@obnoxxx obnoxxx added this to the Release v0.3 milestone Jun 16, 2023
@phlogistonjohn phlogistonjohn removed this from the Release v0.3 milestone Jun 22, 2023
@phlogistonjohn phlogistonjohn force-pushed the jjm-fqin-alt branch 5 times, most recently from 076fdc5 to 98fc5bf Compare July 13, 2023 23:11
@phlogistonjohn phlogistonjohn changed the title [WIP][RFC] build images using new hack/build-image script build images using new hack/build-image script Jul 14, 2023
@phlogistonjohn phlogistonjohn marked this pull request as ready for review July 14, 2023 13:32
@obnoxxx
Copy link
Collaborator

obnoxxx commented Jul 14, 2023

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Jul 14, 2023

rebase

✅ Branch has been successfully rebased

obnoxxx
obnoxxx previously approved these changes Jul 14, 2023
Copy link
Collaborator

@obnoxxx obnoxxx 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 doing this, @phlogistonjohn !

This is a clean improvement of the build mechanisms.

I reviewed the commits, read the new script, and played with it locally.

LGTM.

@obnoxxx
Copy link
Collaborator

obnoxxx commented Jul 14, 2023

@phlogistonjohn needs fixes for failing checks now after rebase

@phlogistonjohn
Copy link
Collaborator Author

Seems like that job might be flaky. This is the 2nd time I just reran it and it worked on the retry.

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

Why do we have the new Python script named build-image and not build-image.py?

Makefile Show resolved Hide resolved
@phlogistonjohn
Copy link
Collaborator Author

Why do we have the new Python script named build-image and not build-image.py?

First, I wanted to ensure that if we ever decided to change how the tool is implemented we wouldn't have to rename it. Also, it's also very common to name executables in python without the .py extension (see commands like black, yamllint, hg, copr-cli, dnf, ansible-playbook and the list goes on). Only python files that expected to be imported need to have a py extension. So I think it's the more typical naming approach for an executable (albeit one that isn't expected to be installed).

Copy link
Collaborator

@synarete synarete left a comment

Choose a reason for hiding this comment

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

Using python script improves code quality and readability, but see review comments.

Makefile Show resolved Hide resolved
hack/build-image Show resolved Hide resolved
hack/build-image Show resolved Hide resolved
hack/build-image Outdated Show resolved Hide resolved
hack/build-image Outdated Show resolved Hide resolved
hack/build-image Outdated Show resolved Hide resolved
hack/build-image Outdated Show resolved Hide resolved
hack/build-image Outdated Show resolved Hide resolved
images/toolbox/Containerfile.centos Show resolved Hide resolved
Add a python-based script `build-image` that takes on the complexities
of how we build our images, including applying common tags and
"fully-qualified image names". The script can be called from a makefile
but doesn't require it. When used directly you can generate
multiple image variants in one pass.
Example:
```
./hack/build-image -k server -p default -p nightly -a amd64 -a arm64
```
Will produce four images, in short:
1. server with default packages on amd64
2. server with default packages on arm64
3. server with nightly packages on amd64
4. server with nightly packages on arm64
(all using the default distro, currently fedora)

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
The "fake" name used in the image only works if you build the container
locally first or are running in our CI. Since this is not a good general
solution, I put a comment here as a reminder it should be fixed later.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@mergify mergify bot dismissed obnoxxx’s stale review July 17, 2023 13:00

Pull request has been modified.

Copy link
Collaborator

@synarete synarete left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

still LGTM. :-)

@mergify mergify bot merged commit 56164e9 into samba-in-kubernetes:master Jul 17, 2023
31 checks passed
@phlogistonjohn phlogistonjohn deleted the jjm-fqin-alt branch July 17, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants