GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat: add multi arch support by kishansairam9 · Pull Request #28 · hypertrace/docker-java-base ConversationUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. All reactions
kishansairam9
commented
|
| pull_request: | ||
|
|
||
| jobs: | ||
| extract-args-java-11: |
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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:
it is not too complex to understand from actions perspective
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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.