-
-
Notifications
You must be signed in to change notification settings - Fork 175
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
chore: use docker bake to generate docker compose file #415
Conversation
1ecdf6d
to
478b69e
Compare
bb9760a
to
53e511a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update https://github.com/jenkinsci/docker-ssh-agent?tab=readme-ov-file#building-and-testing-on-windows as part of this PR please?
Given
The docker compose files are generated from a Linux agent as it avoids tedious yq queries escaping for PowerShell and installing docker buildx on Windows agents.
It looks like you need a lot of additional tools if you are on Windows to be able to build the image (including GNU make
and yq
): am I correct? If yes, wether we agree or not, this PR need the doc. to be updated to guide contributors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really interesting PR technically speaking, showing a lot of different usages and improvements over the current HCL usage.
- There are a lot of tiny improvement which looks really useful without any doubts, such as factorizations to variables in the HCL => do you mind splitting into PR with different goals to make it easier for othger maintainers to review please (I'm not willing to give and approval with so much changes at the same time).
- Only blocker for now is the missing updated documentation for windows contributor: this PR introduces way more requirements for them and it must be documented.
- I'm not sure the end result is easier: it goes on the opposite style as what we used to do: trying to remove the abstraction of Make/make.ps1 . I won't block for this as it is philosophical
- Could you explain what is the "problem" you want to solve with this PR? As much as it looks interesting and fun technically, I fail to see the gain
The main problem I have with the current status is that we don't have a "pure" definition of Windows images with the docker compose file as it is, we need to massage data with build.ps1 in order to obtain the correct value for TOOLS_WINDOWS_VERSION for example. This PR proposes having "pure" images definitions, inside a single common source of truth (the bake file), containing all the helper functions (without having to replicate them in build.ps1) and all the parameters needed to generate the images. It also paves the way for when docker build bake will be usable to build Windows images. |
dd78325
to
7bcc98f
Compare
I reworked the PR to generate the docker compose file from a Windows agent instead of a Linux one, As
Extracted in:
I've detailed everything in #426.
Currently working on it, the rest is in 7bcc98f I'll pass the PR in ready for review when I'll add the documentation. |
7bcc98f
to
54d6230
Compare
54d6230
to
ed751ae
Compare
Pull request ready for review, this commit especially: f93da6a?w=1 The rest are cherry-picks from the other pull requests, amongst those #419 and #424 are required to be merged before merging this one. Or eventually, this PR can also be merged as it is, superseding all extracted PRs if they're OK for maintainers. |
4c2d2ba
to
f93da6a
Compare
Resulting build.ps1 "main" scriptLines 186 to 258 in e93d536
EDIT: droped, can't make it work on ci.jenkins.io while it works locally, will try later. |
c74738d
to
f93da6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good \o/
Devil in the details; I've added a few nitpicks or non blocking comments, I let you decide if you want to keep them or not.
Great job and documentation!
Jenkinsfile
Outdated
} else { | ||
powershell "& ./build.ps1 -VersionTag ${env.TAG_NAME} publish" | ||
powershell '& ./build.ps1 publish' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: I'm transplanting the comments in #421
As the cherry-picked commits and the corresponding (merged) pull requests have diverged, I'm rebasing this pull request into a clean commit. |
13d6e93
to
a305ab1
Compare
This PR uses docker bake to generate the content of the docker compose files for Windows images from the same docker-bake.hcl used for Linux images.
Fixes:
The JSON output of
docker buildx bake windows --print
is converted withyq
into the YAML format expected by docker compose to build the corresponding images. (We can't use bake for building as it's not functional on Windows yet.)The docker compose files are generated (here) from a Linux agent as it avoids tediousyq
queries escaping for PowerShell and installing docker buildx on Windows agents.EDIT:
As
docker buildx bake windows --print
can be used on Windows,make
isn't required for contributors.And as
yq
is already required, the only new requirement for contributors is https://github.com/docker/buildx included in Docker Desktop, one of the most common docker installation types on Windows.Example of resulting build-windows.yaml (note that no arg depends on any variable)
Obtained with:
Note: this command works for Linux/MacOS, Windows (or more specifically PowerShell) requires backslashes in front of every double quote and setting environment variables on their own.
Enhancements:
Images content remains untouched hence the "chore" label.
Testing done
Local tests and CI.
Submitter checklist