Skip to content

Update download-hz-dist to support multiple repositories [DI-730]#67

Open
JackPGreen wants to merge 3 commits into
masterfrom
DI-730
Open

Update download-hz-dist to support multiple repositories [DI-730]#67
JackPGreen wants to merge 3 commits into
masterfrom
DI-730

Conversation

@JackPGreen

@JackPGreen JackPGreen commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

As part of the groundwork to support DI-730, improve download-hz-dist:

  • support multiple repositories
    • currently it only supports a single repository at a time, hence we have to supply a repository for each configuration (release/SNAPSHOT/OSS/EE etc)
    • it would be better to supply a set of repositories and have it figure out where to find the artifact
  • centralise repositories
    • the repository configuration in hazelcast-docker and hazelcast-packaging is duplicated in each GitHub repo environment
    • it would be better if this was in a single place
    • passing all the repo vars is messy
  • migrate away from a deprecated maven-dependency-plugin
    • replace mvn with curl
    • see DI-663

@JackPGreen JackPGreen force-pushed the DI-730 branch 11 times, most recently from 9d6ff64 to 643b865 Compare June 22, 2026 19:08
@JackPGreen JackPGreen changed the title DI-730 Refactor download-hz-dist to support multiple repositories Jun 22, 2026
@JackPGreen JackPGreen changed the title Refactor download-hz-dist to support multiple repositories Refactor download-hz-dist to support multiple repositories [DI-730] Jun 22, 2026
@JackPGreen JackPGreen changed the title Refactor download-hz-dist to support multiple repositories [DI-730] Update download-hz-dist to support multiple repositories [DI-730] Jun 22, 2026
@JackPGreen JackPGreen marked this pull request as ready for review June 22, 2026 19:20
Comment thread .github/workflows/test-download-hz-dist.yml Outdated
Comment thread download-hz-dist/action.yml Outdated
echo "url=${url}" >> ${GITHUB_OUTPUT}
exit 0
else
echodebug "Failed to download from ${url}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not a failure but just not found.
otherwise little misleading!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well it could be a failure... all it really means is curl returned a non-0 exit code. But you won't be able to see that, because --silent...

fi
done <<< "${{ steps.derive-repositories.outputs.repositories }}"

echoerr "Distribution not found in ${{ steps.derive-repositories.outputs.repositories }}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
echoerr "Distribution not found in ${{ steps.derive-repositories.outputs.repositories }}"
echoerr "Distribution '${{ steps.derive-output-file.outputs.file }}' not found in ${{ steps.derive-repositories.outputs.repositories }}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's not accurate as that's a user input (i.e. they could set the output file to anything). You could set it to inputs.distribution, but then you'd want to see the version etc and it just gets confusing.

case "${ENVIRONMENT}" in
live)
repos=$(cat <<'EOF'
https://repo.maven.apache.org/maven2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not sure here but can we say whats the more frequent use case i.e. snapshots or release?
the list should favor that to minimize curl misses
we also do more PATCH releases so may be central should be last

not a big deal but thought I mention

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's ordered alphabetically. I don't know what ordering would be more efficient, I did think about doing it in parallel but if more than one was successful (i.e. preprod + live) it'd overwrite the same file and be a mess so just left it.

Comment thread get-supported-platforms/action.yml Outdated
;;
*)
echoerr "Unsupported platform behaviour for ${base_image_name}" ; return 1
echoerr "Unsupported platform behaviour for ${base_image_name}" ; exit 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You return from a function, and exit from a shell - it's not in a function, so we shouldn't return.
I copied this switch when adding the repo switch statement to the new action and spotted it so thought I'd fix it there too.

JackPGreen and others added 2 commits June 23, 2026 14:52
Co-Authored-By: Nishaat Rajabali <12186256+nishaatr@users.noreply.github.com>
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants