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

[Backport] [2.x] Fix GA workflow that publishes Apache Maven artifacts (#2625) #2648

Merged
merged 1 commit into from
Jul 12, 2024
Merged

[Backport] [2.x] Fix GA workflow that publishes Apache Maven artifacts (#2625) #2648

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/maven-publish.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:
- uses: actions/setup-java@v3
with:
distribution: temurin # Temurin is a distribution of adoptium
java-version: 17
java-version: 11
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use JDK-11 baseline for 2.x

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see this PR https://github.com/opensearch-project/ml-commons/pull/2625/files using java 21
So 2.x doesn't support 21?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, and we build 2.x with 11 / 17 / 21, but main (3.x) is different- it has 21 baseline and the minimum JDK it can be built is 21, thanks @ylwu-amzn

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool, so this is for minimum JDK version ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, 11 is better in this case - we are 100% the produced artifacts are compliant

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I have a confusion here:

We usually merge code in Main branch first, right? Let's say, I added a method in my code which works only in JDK 21 and I merged the code in main and then found the issue while backporting this to 2.x branch?

What will be my action item? Finding a suitable method which works for jdk 11 too? Or skip the integ failure for 11 & 17?

This doesn't seem like developer friendly experience to me?

@reta do you have any suggestion on this?

Copy link
Contributor Author

@reta reta Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @dhrubo-os

We usually merge code in Main branch first, right? Let's say, I added a method in my code which works only in JDK 21 and I merged the code in main and then found the issue while backporting this to 2.x branch?

Yes

What will be my action item? Finding a suitable method which works for jdk 11 too? Or skip the integ failure for 11 & 17?

Yes, you have to change a code to be compatible with JDK-11 baseline

This doesn't seem like developer friendly experience to me?

You are free to use only JDK-11 compatible APIs in main which basically means you won't be able to use any new language or library features. This is 100% your (== maintainers) choice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@reta Thanks for your reply.

You are free to use only JDK-11 compatible APIs in main which basically means you won't be able to use any new language or library features.

Yeah exactly. In that case what's the point of using JDK 21 baseline, if I have to use JDK 11 feature/compatible apis eventually?

This is 100% your (== maintainers) choice.

I'm curious to know what do you guys do in Core? What's the best practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah exactly. In that case what's the point of using JDK 21 baseline, if I have to use JDK 11 feature/compatible apis eventually?

@dhrubo-os the motivation for the change itself is provided in the issue (opensearch-project/OpenSearch#10745) that is linked to ml-commons one. TLDR; it is driven by components we depend upon.

I'm curious to know what do you guys do in Core? What's the best practice?

We don't have this experience yet (the core will be the last in line to update otherwise we'll break the builds of the plugins / components that have not been moved to JDK-21 baseline yet). But every single project which I am aware of is doing some work while backporting to the older maintenance branches: the work is either manual or automated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, thanks for your explanation.

- uses: actions/checkout@v3
- uses: aws-actions/[email protected]
with:
Expand Down
Loading