GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat: add multi arch support by kishansairam9 · Pull Request #28 · hypertrace/docker-java-base

Conversation

@kishansairam9

@kishansairam9 kishansairam9 commented Feb 6, 2024

Copy link
Copy Markdown

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
kishansairam9
feat: add multi arch support for java 21
5032c57
@kishansairam9
kishansairam9 requested review from aaron-steinfeld and ravisingal February 6, 2024 10:12
kishansairam9
update 11 too
0664dda
@kishansairam9 kishansairam9 changed the title feat: add multi arch support for java 21 feat: add multi arch support Feb 6, 2024
aaron-steinfeld
aaron-steinfeld reviewed Feb 6, 2024
View reviewed changes
Comment thread java-11/Dockerfile Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread java-11/Dockerfile Resolved

Uh oh!

There was an error while loading. Please reload this page.

@kishansairam9
kishansairam9 requested a review from aaron-steinfeld February 7, 2024 16:03
@kishansairam9 kishansairam9 mentioned this pull request Feb 7, 2024
expose platforms and apply spotless hypertrace/hypertrace-gradle-docker-plugins#38
Closed
Kishan Sairam Adapa
Merge branch 'main' into multi-arch
ca44e1f
aaron-steinfeld
aaron-steinfeld reviewed Feb 7, 2024
View reviewed changes
Comment thread java-21/Dockerfile

# set JAVA_HOME
ENV JAVA_HOME=/usr/lib/jvm/zulu-21-amd64-slim
ENV JAVA_HOME=/usr/lib/jvm/zulu-21-slim

@aaron-steinfeld aaron-steinfeld Feb 7, 2024

Copy link
Copy Markdown
Contributor

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 change alone isn't really changing anything is it? Since no one is building the base docker images, and the CI job here hasn't been changed we're still going to end up only publishing the amd64 version right?

If I'm following that correctly, can we update to publish this as multiarch too?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kishansairam9 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
kishansairam9 added 3 commits February 7, 2024 23:55
kishansairam9
remove gradle, use new actions
0c36b58
kishansairam9
Merge remote-tracking branch 'origin/main' into multi-arch
85a637d
kishansairam9
Merge remote-tracking branch 'origin/multi-arch' into multi-arch
686e76b
@kishansairam9
kishansairam9 requested review from JBAhire and kotharironak as code owners February 7, 2024 18:26
kishansairam9 added 15 commits February 7, 2024 23:57
kishansairam9
nit
d85b19b
kishansairam9
update
463de30
kishansairam9
update
eb876da
kishansairam9
checkout
e855aad
kishansairam9
try again
30e298c
kishansairam9
try again
2cd826b
kishansairam9
fix dep
67cb346
kishansairam9
blunder
f6b0cfc
kishansairam9
check
e035f4a
kishansairam9
check
8515f63
kishansairam9
delimit
1609699
kishansairam9
test
483b25b
kishansairam9
test
e7b7fab
kishansairam9
test
27e6eeb
kishansairam9
test
b709112
kishansairam9
final
5138652
@kishansairam9
kishansairam9 requested a review from aaron-steinfeld February 7, 2024 19:25
kishansairam9
kishansairam9 commented Feb 7, 2024
View reviewed changes
Comment thread .github/workflows/extract-args.yml Outdated
echo "tags=${{ inputs.image }}:$MAJOR_JAVA_VERSION,${{ inputs.image }}:$BASE_JAVA_VERSION,${{ inputs.image }}:$TOTAL_JAVA_VERSION" >> $GITHUB_OUTPUT
{
echo "args<<nEOFn"
echo "$(cat ${{ inputs.file }})"

@kishansairam9 kishansairam9 Feb 7, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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.

wasted too much time here because of not wrapping this in echo not creating new line as it is not in file and leading to EOF not found 😔

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
kishansairam9 added 4 commits February 8, 2024 01:00
kishansairam9
load
2c4dfe9
kishansairam9
change name
f386295
kishansairam9
load
f510a92
kishansairam9
use matrix as cannot load multiplatform
5f2f680
aaron-steinfeld
aaron-steinfeld reviewed Feb 7, 2024
View reviewed changes
Comment thread .github/workflows/pr-build.yml Outdated
pull_request:

jobs:
extract-args-java-11:

@aaron-steinfeld aaron-steinfeld Feb 7, 2024

Copy link
Copy Markdown
Contributor

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 seems like excessive complexity for a simple task. In the build case, we don't even need to generate all the individual tags anyway. In the publish version, since that's the only spot we need them, you can just inline the couple cut calls.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@kishansairam9 kishansairam9 Feb 7, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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.

Yes it is a bit complex, but since we wouldn't want to store build args hidden in github actions yamls rather in repo source I thought this is required

Can we inline reading data from a file and doing cut to pass as a variable to an action? I couldn't think of a way to do it and built a reusable workflow. I reused the same in build too even if isn't strictly required

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aaron-steinfeld aaron-steinfeld Feb 7, 2024

Copy link
Copy Markdown
Contributor

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 do something like this? basically just an inline form of the same code. I also moved the build arg name from inside the file to outside.

      - name: Read Java Version
        shell: bash
        id: versions
        run: |
          ZULU_JDK_VERSION=`cat ${{ matrix.java-major-version}}/java.version}}`
          echo ZULU_JDK_VERSION=${ZULU_JDK_VERSION} >> $GITHUB_OUTPUT
          echo JAVA_MAJOR_VERSION=$(echo $ZULU_JDK_VERSION | cut -d'.' -f1)" >> $GITHUB_OUTPUT
          echo JAVA_FULL_VERSION=$(echo $ZULU_JDK_VERSION | cut -d'-' -f1) >> $GITHUB_OUTPUT

Then, for build args just declare it like

build-args: |
                      JAVA_VERSION=${{steps.versions.output.ZULU_JDK_VERSION}}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kishansairam9 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@aaron-steinfeld aaron-steinfeld Feb 7, 2024

Copy link
Copy Markdown
Contributor

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.

I also wouldn't bother publishing the zulu version tag - it should be treated as an impl detail since we're not making any guarantees about the specific distro of java we're using.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@kishansairam9 kishansairam9 Feb 7, 2024

Copy link
Copy Markdown
Author

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 fine by me, I wanted to leave scope for any more build args being added in future without need to touch gha

Not a strong opinion, but would prefer to have the current args.env file version as:

  1. it is not too complex to understand from actions perspective

  2. decent tradeoff of complexity in a gha yaml file which will be rarely looked upon for the benefit of not visiting gha part when someone adds a new build arg (which I agree will be rare, but so is someone trying to understand this reusable workflow)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aaron-steinfeld aaron-steinfeld Feb 7, 2024

Copy link
Copy Markdown
Contributor

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.

I'm not sure why we'd add any more args, and if we did, it's likely we'd be changing the build too. If the arg was statically defined, there'd be no point in passing it in in the first place. If it were dynamic, we'd likely be using a matrix or some other way of calculating the value for it in CI then need to pass them in like above anyway.

The java version is actually bit of a strange case as it's static, but kept outside the dockerfile so it can be consumed to tag as well. A common counter example would be something like the current commit SHA.

So in other words, the more likely future case is we'd need to calculate a build arg in the build and then pass it to the build command which would be more difficult to do if merging it with the static env file. Not positive if that makes sense, or if I'm missing your point though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@kishansairam9 kishansairam9 Feb 8, 2024

Copy link
Copy Markdown
Author

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.

The java version is actually bit of a strange case as it's static, but kept outside the dockerfile so it can be consumed to tag as well.

Ah, got it. I thought it was done as a good practice whatever things are not required to be static, make them as args and pass it during build. Hence I was thinking of multiple build args as a possibility.

Makes sense to me now, updating

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread .github/workflows/pr-build.yml Resolved

Uh oh!

There was an error while loading. Please reload this page.

kishansairam9
update
d503643
@kishansairam9
kishansairam9 requested a review from aaron-steinfeld February 8, 2024 03:52
kishansairam9 added 3 commits February 8, 2024 09:24
kishansairam9
bug
2efd76f
kishansairam9
bug
51836e6
kishansairam9
bug
0e64d58
@hypertrace hypertrace deleted a comment from kishansairam9 Feb 8, 2024
aaron-steinfeld
aaron-steinfeld approved these changes Feb 8, 2024
View reviewed changes
@kishansairam9
kishansairam9 merged commit 701e3c3 into main Feb 8, 2024
@kishansairam9
kishansairam9 deleted the multi-arch branch February 8, 2024 13:41
@GautamMPanchaL GautamMPanchaL mentioned this pull request Oct 29, 2026
Bump Java 21 Zulu build to 21.0.13.0.3 (zulu21.54.109) #60
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aaron-steinfeld aaron-steinfeld aaron-steinfeld approved these changes
@ravisingal ravisingal Awaiting requested review from ravisingal
@JBAhire JBAhire Awaiting requested review from JBAhire JBAhire is a code owner
@kotharironak kotharironak Awaiting requested review from kotharironak kotharironak is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@kishansairam9 @aaron-steinfeld
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.