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

build: test with OpenJDK 17 #1219

Merged
merged 2 commits into from
Sep 8, 2023
Merged

Conversation

johnshajiang
Copy link
Contributor

Now that jetcd should support OpenJDK 11+, it would be better to test not only OpenJDK 11, but OpenJDK 17 as well.
For example, the issue fixed by #1216 can only be reproduced with OpenJDK 13+.

Signed-off-by: John Jiang <john.sha.jiang@gmail.com>
@liangyuanpeng
Copy link
Contributor

I think we can waiting for java 21.

@@ -42,6 +42,7 @@ jobs:
strategy:
fail-fast: false
matrix:
java-version: [11, 17]
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did think about this point.
Frankly, I was not sure what are the purposes of build-main.yml and build-tag.yml.
So, I focus on PR only. And that is why the title is test with OpenJDK 17 on PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

build tag was an attempt to automate the release process but it is not yet working

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated build-main.yml and added OpenJDK 17 for build job only.

@johnshajiang
Copy link
Contributor Author

@liangyuanpeng

I think we can waiting for java 21.

Not sure when Gradle can support JDK 21.
Please monitor gradle/gradle#25574
In fact, it needs to upgrade Gradle to 8.3 for supporting JDK 20.

Signed-off-by: John Jiang <john.sha.jiang@gmail.com>
@johnshajiang johnshajiang changed the title build: test with OpenJDK 17 on PR build: test with OpenJDK 17 Sep 7, 2023
@johnshajiang
Copy link
Contributor Author

@lburgazzoli
Could you please review this PR?
My following PR for issue #1221 will modify build-pr.yml as well.

Please feel free to give your suggestions or concerns.

@lburgazzoli lburgazzoli merged commit 0450dac into etcd-io:main Sep 8, 2023
6 checks passed
@johnshajiang johnshajiang deleted the test-on-jdk17 branch September 8, 2023 06:51
@johnshajiang
Copy link
Contributor Author

@lburgazzoli
Thanks for approving and merging this PR!

In addition, I just have a suggestion.
It may be better to squash the multiple commits into a single commit when merge this PR.

@lburgazzoli
Copy link
Collaborator

@johnshajiang I usually do ask the author to squash the commits as the author knows better. In this case I could probably have done it but that leaves to me the role of deciding the commit message and the description which may lead to remove some useful context the author wanted to preserve.

@johnshajiang
Copy link
Contributor Author

@lburgazzoli
Please feel free to ask me to squash the commits after you finally approve my PR.

@lburgazzoli
Copy link
Collaborator

@lburgazzoli Please feel free to ask me to squash the commits after you finally approve my PR.

certainly, it was also very hard to get someone committed to do some good work so request to get reproducers, fix stuffs in PR and what not has always been a little bit not easy to achieve so as result I take the easy path and I fix things later on when I have time so any active contribution is more than welcome :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants