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

Update Gradle to 8.10 #1140

Merged
merged 2 commits into from
Aug 15, 2024
Merged

Update Gradle to 8.10 #1140

merged 2 commits into from
Aug 15, 2024

Conversation

reta
Copy link
Collaborator

@reta reta commented Aug 15, 2024

Description

Update Gradle to 8.10

Issues Resolved

N/A_

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@reta reta added backport 2.x Backport to 2.x branch and removed backport 2.x Backport to 2.x branch labels Aug 15, 2024
dblock
dblock previously approved these changes Aug 15, 2024
@dblock
Copy link
Member

dblock commented Aug 15, 2024

The unreleased opensearch version integ tests need a different JDK like in opensearch-project/opensearch-py#798, want to fix it here? Also the backport workflow seems off.

Signed-off-by: Andriy Redko <[email protected]>
@reta
Copy link
Collaborator Author

reta commented Aug 15, 2024

The unreleased opensearch version integ tests need a different JDK like in opensearch-project/opensearch-py#798, want to fix it here?

Sure, thanks @dblock

Also the backport workflow seems off.

Ah, that's like that: label should be after the merge

@dblock
Copy link
Member

dblock commented Aug 15, 2024

Ah, that's like that: label should be after the merge

Should probably not run on non-merged PRs then?

@@ -48,10 +48,22 @@ jobs:
path: opensearch/distribution/archives/linux-tar/build/distributions
key: ${{ steps.get-key.outputs.key }}

- name: Set up JDK 17
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but maybe a cleaner/simpler implementation adds a java-runtime to the matrix above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we need 2 JDKs to be installed in case of 11: 11 + 17, and only one in all other cases. Adding java-runtime would lead to duplicate actions/setup-java@v4 runs, isn't it?

Copy link
Member

@dblock dblock Aug 15, 2024

Choose a reason for hiding this comment

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

Like this:

      matrix:
        entry:
          - { opensearch_ref: '1.x', java: 11 }
          - { opensearch_ref: '2.x', java: 11, java-runtime: 17 }
          - { opensearch_ref: '2.x', java: 17 }
          - { opensearch_ref: '2.x', java: 21 }
          - { opensearch_ref: 'main', java: 11, java-runtime: 17 }
          - { opensearch_ref: 'main', java: 17 }
          - { opensearch_ref: 'main', java: 21 }

Then below you do if: matrix.java-runtime and java-version: ${{ matrix.java-runtime }}.

This makes the intent very clear.

Copy link
Collaborator Author

@reta reta Aug 15, 2024

Choose a reason for hiding this comment

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

But you still need this if statement :( Let me try, one sec

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks better @dblock ?

dblock
dblock previously approved these changes Aug 15, 2024
@reta
Copy link
Collaborator Author

reta commented Aug 15, 2024

Should probably not run on non-merged PRs then?

That's why it fails with the message that it should be added on merged prs

Signed-off-by: Andriy Redko <[email protected]>
@dblock dblock merged commit 7d894d6 into opensearch-project:main Aug 15, 2024
56 checks passed
@reta reta added the backport 2.x Backport to 2.x branch label Aug 15, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 15, 2024
* Update Gradle to 8.10

Signed-off-by: Andriy Redko <[email protected]>

* Address code review comments

Signed-off-by: Andriy Redko <[email protected]>

---------

Signed-off-by: Andriy Redko <[email protected]>
(cherry picked from commit 7d894d6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this pull request Aug 15, 2024
* Update Gradle to 8.10



* Address code review comments



---------


(cherry picked from commit 7d894d6)

Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@reta reta mentioned this pull request Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants