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

fix(alpine) The Alpine Dockerfile does not use the ALPINE_TAG arg #421

Merged
merged 6 commits into from
May 19, 2023

Conversation

gounthar
Copy link
Contributor

@gounthar gounthar commented May 17, 2023

@MarkEWaite detected in #415 that there was a mismatch in the naming of our Alpine Docker image, and solved it in #416.
@dduportal took some time to guide me in figuring out the problem may need a more permanent fix.

We are getting the Alpine version shipped with Eclipse Temurin. We don't take into account the ALPINE_TAG set in the docker-bake.hcl file. Damien's proposal was to do as we do with Debian, proceed with two stages.
One stage to get the Temurin build, and one to get the correct (i.e. defined in the docker-bake) Alpine version.
The (optional?) next step would be to use jlink as we do in the Debian image.

Testing done

The only testing I have done is run locally after building the image:

docker run --rm jenkins/agent:alpine-jdk17 cat /etc/os-release
NAME="Alpine Linux"
ID=alpine
VERSION_ID=3.17.3
PRETTY_NAME="Alpine Linux v3.17"
HOME_URL="https://alpinelinux.org/"
BUG_REPORT_URL="https://gitlab.alpinelinux.org/alpine/aports/-/issues"

and

docker run --rm jenkins/agent:alpine-jdk17 java --version
openjdk 17.0.7 2023-04-18
OpenJDK Runtime Environment Temurin-17.0.7+7 (build 17.0.7+7)
OpenJDK 64-Bit Server VM Temurin-17.0.7+7 (build 17.0.7+7, mixed mode, sharing)

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

…o build

We are getting the Alpine version shipped with Eclipse Temurin.
We don't take into account the `ALPINE_TAG` set in the `docker-bake.hcl` file.
Damien's proposal was to do as we do with Debian, with two stages, one to get the Temurin build, and one to get the correct (i.e. defined in the `docker-bake`) Alpine version.
The (optional?) next step would be to use `jlink` as in the Debian image.
The part about `arm` is not necessary for the time being, as there is no version of Temurin's openJDK build for Alpine on arm.
adoptium/adoptium#224
It won't happen tomorrow either, but we can keep faith for Java 21.
@dduportal
Copy link
Contributor

Nice job buddy! It looks like it's working.

I'ts almost ready to merge!

I see two last steps (which we did not discuss before):

@gounthar gounthar marked this pull request as ready for review May 19, 2023 09:54
@gounthar gounthar requested a review from a team as a code owner May 19, 2023 09:54
cp -r /opt/java/openjdk /javaruntime; \
fi

ARG ALPINE_TAG=3.17.3
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
ARG ALPINE_TAG=3.17.3

As the ALPINE_TAG build argument is only consumed by the FROM alpine ... AS build instruction, it's only needed once, before any FROM instruction as per the Dockerfile ARG scope documentation.

Yeah I know it's a bit weird, and this change is not blocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is I have an error when removing the first ARG.

docker buildx bake alpine_jdk17 --load
[+] Building 0.2s (3/3) FINISHED
 => [internal] load .dockerignore                                                                                                                                             0.1s
 => => transferring context: 2B                                                                                                                                               0.1s
 => [internal] load build definition from Dockerfile                                                                                                                          0.0s
 => => transferring dockerfile: 3.81kB                                                                                                                                        0.0s
 => CANCELED [internal] load metadata for docker.io/library/eclipse-temurin:17.0.7_7-jdk-alpine                                                                               0.0s
Dockerfile:43
--------------------
  41 |
  42 |     ARG ALPINE_TAG=3.17.3
  43 | >>> FROM alpine:"${ALPINE_TAG}" AS build
  44 |
  45 |     ARG user=jenkins
--------------------
ERROR: failed to solve: failed to parse stage name "alpine:": invalid reference format

Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

I've added a non blocking comment: we can merge now or add my suggestion (and fix the comment later): your call :)

@dduportal dduportal merged commit 8967e06 into jenkinsci:master May 19, 2023
@gounthar
Copy link
Contributor Author

LGTM thanks!

I've added a non blocking comment: we can merge now or add my suggestion (and fix the comment later): your call :)

Sorry, I did not react fast enough.
I'll add it to the next PR if you don't mind.

@dduportal
Copy link
Contributor

LGTM thanks!
I've added a non blocking comment: we can merge now or add my suggestion (and fix the comment later): your call :)

Sorry, I did not react fast enough. I'll add it to the next PR if you don't mind.

No problem buddy, it's full async and my comment was not blocking. I prefer shipping "fast" (but without rush from the author :) ) tiny PRs and release often

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.

2 participants