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.

Sign up for GitHub

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

Jump to bottom

Docker develop publishing task #6893

Merged
merged 5 commits into from
Apr 15, 2024
Merged

Docker develop publishing task #6893

merged 5 commits into from
Apr 15, 2024

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Apr 6, 2024 •
edited
Loading

PR description

Add a minimal github action to publish develop docker containers on merge to main.

The task will publish one develop image per arch, and a multi-arch manifest.
e.g. the images that this task will publish on every merge to main (currently) are:

  • develop multi-arch
  • develop-arm64 (aarch64)
  • develop-amd64 (amd64)

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@garyschulte garyschulte requested review from jflo and siladu April 6, 2024 00:10
usmansaleem
usmansaleem reviewed Apr 8, 2024
View reviewed changes
Copy link
Member

@usmansaleem usmansaleem left a comment

Choose a reason for hiding this comment

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

Justin has modified the build to only use one jdk i.e. jdk 21. This gh action may be required to be updated.

@garyschulte
initial docker develop task
aaef3d8 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
use/lint only single Dockerfile
afb8df5 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte garyschulte force-pushed the feature/develop-docker-image branch from aebae3f to afb8df5 Compare April 11, 2024 17:41
@garyschulte
Copy link
Contributor Author

Justin has modified the build to only use one jdk i.e. jdk 21. This gh action may be required to be updated.

Thanks for the heads up. Rebasing after the merge of @jflo 's docker changes, I removed the explicit docker variant dockerfile linting, and tested on my fork. We now produce just 3 develop images on a merge to main:

develop (multi-arch)
develop-arm64
develop-amd64

usmansaleem
usmansaleem reviewed Apr 11, 2024
View reviewed changes
.github/workflows/develop.yml
# Construct the build target name
BUILD_TARGET_NAME="${DATE_TIME}-develop-${SHORT_SHA}"
echo "Build Target Name: $BUILD_TARGET_NAME"
# Set the build target name as an environment variable
Copy link
Member

Choose a reason for hiding this comment

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

is there meant to be an export command here based on the comment?

usmansaleem
usmansaleem approved these changes Apr 11, 2024
View reviewed changes
Copy link
Member

@usmansaleem usmansaleem left a comment

Choose a reason for hiding this comment

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

LGTM, minor comment.

siladu
siladu approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Couple of non-blocking comments. I took the liberty of making a one character change :)

.github/workflows/develop.yml
# Get the current date and time in the format YY.MM
DATE_TIME=$(date +"%y.%m")
# Get the short SHA of the merge commit
SHORT_SHA=${GITHUB_SHA::7}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this equivalent to

besu/.github/workflows/docker.yml

Lines 53 to 55 in ec06ab5

- name: short sha
id: shortSha
run: echo "sha=$(git rev-parse --short HEAD)" >> $GITHUB_OUTPUT

or is SHORT_SHA only used internally here?

If the latter, then why don't we also need the set the sha variable in the same way docker.yml does?

build.gradle
@@ -979,6 +979,7 @@ def getGitCommitDetails(length = 8) {
def isInterimBuild(dockerBuildVersion) {
return (dockerBuildVersion ==~ /.*-SNAPSHOT/) || (dockerBuildVersion ==~ /.*-alpha/)
|| (dockerBuildVersion ==~ /.*-beta/) || (dockerBuildVersion ==~ /.*-RC.*/)
|| (dockerBuildVersion ==~ /.*develop.*/)
Copy link
Contributor

Choose a reason for hiding this comment

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

comment above is out of date now :)

@siladu siladu enabled auto-merge (squash) April 15, 2024 04:44
@siladu siladu merged commit b67ad69 into hyperledger:main Apr 15, 2024
42 checks passed
@garyschulte garyschulte deleted the feature/develop-docker-image branch April 15, 2024 14:48
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
@garyschulte @siladu @amsmota
Docker develop publishing task (hyperledger#6893)
61471cf 
Publish to dockerhub on merge to the main branch

Signed-off-by: garyschulte <garyschulte@gmail.com>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: amsmota <antonio.mota@citi.com>
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
@garyschulte @siladu @amsmota
Docker develop publishing task (hyperledger#6893)
4f23808 
Publish to dockerhub on merge to the main branch

Signed-off-by: garyschulte <garyschulte@gmail.com>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: amsmota <antonio.mota@citi.com>
macfarla pushed a commit to macfarla/besu that referenced this pull request Apr 26, 2024
@garyschulte @siladu @macfarla
Docker develop publishing task (hyperledger#6893)
1aed55d 
Publish to dockerhub on merge to the main branch

Signed-off-by: garyschulte <garyschulte@gmail.com>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
matthew1001 pushed a commit to kaleido-io/besu that referenced this pull request Jun 7, 2024
@garyschulte @siladu @matthew1001
Docker develop publishing task (hyperledger#6893)
0791a78 
Publish to dockerhub on merge to the main branch

Signed-off-by: garyschulte <garyschulte@gmail.com>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siladu siladu siladu approved these changes

@usmansaleem usmansaleem usmansaleem approved these changes

@jflo jflo Awaiting requested review from jflo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@garyschulte @usmansaleem @siladu @macfarla