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

Re-tag images after release mark ready #3842

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

farodin91
Copy link
Contributor

@farodin91 farodin91 commented Jun 17, 2023

Part of #3793


Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

@farodin91 farodin91 added this to the Release v1.0.0 milestone Jun 17, 2023
@farodin91 farodin91 marked this pull request as ready for review June 17, 2023 12:29
@janusgraph-bot janusgraph-bot added the cla: external Externally-managed CLA label Jun 17, 2023
@farodin91 farodin91 force-pushed the on-release branch 2 times, most recently from 92cc34a to 95e4382 Compare June 17, 2023 12:45
@farodin91
Copy link
Contributor Author

@FlorianHockmann Would you like to review?

Copy link
Member

@FlorianHockmann FlorianHockmann left a comment

Choose a reason for hiding this comment

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

The changes look good to me in general, but I'm not sure I understand the release process completely.

So the usual workflow is now that we will push images tagged as 1.0.0-SNAPSHOT and 1.0.0-SNAPSHOT-[commit-hash] from master (and similarly for v0.6). This makes sense of course.

Once we create a release, it will also add the tags: 1.0, 1.0.0, and (if applicable) latest. Will this action also already push the tags? (I wasn't sure from the description of the workflow.)

And after we have created the PR that updates the version on the release branch from 1.0.0-SNAPSHOT to 1.0.0, won't any follow-up commit already push the images with 1.0.0?

@farodin91 farodin91 force-pushed the on-release branch 2 times, most recently from 8fe5665 to 1ad4d0f Compare July 6, 2023 11:03
@farodin91
Copy link
Contributor Author

@FlorianHockmann It should be updated with all discussed changes.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 31, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: farodin91 / name: Jan Jansen (94dda13)

@farodin91
Copy link
Contributor Author

@FlorianHockmann @porunov This should make janusgraph-docker migration complete without the improved documentation yet and fixes the janusgraph-dist build.

Copy link
Member

@porunov porunov left a comment

Choose a reason for hiding this comment

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

Thank you @farodin91 for figuring out how to fix #3894 .
I still don't understand what changed in ubuntu-20.04 that causes the tests to fail now.
This could potentially be the root cause of the issue #1826.
I just think that overtime after some updates the tests might stop working for ubuntu-22.04. I guess we need to open a separate ticket which is related to the failing tests investigation on ubuntu-20.04. Nevertheless, it will be out of scope of this PR I think.

The PR looks good to me, but I'm confused of overall Docker deployment process.

I see the next line which (I think) suppose to deploy docker container:

<id>docker-deploy</id>

Now you are adding the new file .github/workflows/docker-release.yml. What is this file doing?

.github/workflows/docker-release.yml Outdated Show resolved Hide resolved
.github/workflows/docker-release.yml Outdated Show resolved Hide resolved
.github/workflows/docker-release.yml Outdated Show resolved Hide resolved
Copy link
Member

@porunov porunov left a comment

Choose a reason for hiding this comment

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

Thank you @farodin91 for working on this. Even so I left some nitpicks they are up to you to resolve them in this PR or not (i.e. we can resolve them in other PRs as well).
Other than those nitpicks the changes look good to me.

Copy link
Member

@FlorianHockmann FlorianHockmann left a comment

Choose a reason for hiding this comment

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

LGTM, but it would be good if you could address @porunov's questions / comments.

@farodin91
Copy link
Contributor Author

@porunov I fixed the nitpicks.

@farodin91 farodin91 merged commit 8e3816f into JanusGraph:master Aug 4, 2023
170 checks passed
@janusgraph-automations
Copy link

💔 All backports failed

Status Branch Result
v0.6 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

backport --pr 3842

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

@farodin91
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
v0.6

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.6 cla: external Externally-managed CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants