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

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

Merged

Conversation

reta
Copy link
Contributor

@reta reta commented Jul 12, 2024

Description

Fix GA workflow that publishes Apache Maven artifacts

Issues Resolved

Backport of #2625 to 2.x

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

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 temporarily deployed to ml-commons-cicd-env July 12, 2024 12:10 — with GitHub Actions Inactive
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 12:10 — with GitHub Actions Inactive
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 12:10 — with GitHub Actions Inactive
@reta reta had a problem deploying to ml-commons-cicd-env July 12, 2024 12:10 — with GitHub Actions Failure
@reta reta changed the title Fix GA workflow that publishes Apache Maven artifacts (#2625) [Backport] [2.x] Fix GA workflow that publishes Apache Maven artifacts (#2625) Jul 12, 2024
@reta reta force-pushed the fix.maven.publish.workflow.2.x.1 branch from 1dff6c1 to 8d69d7a Compare July 12, 2024 12:16
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 12:16 — with GitHub Actions Inactive
@reta reta had a problem deploying to ml-commons-cicd-env July 12, 2024 12:16 — with GitHub Actions Failure
@@ -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

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.

@reta reta force-pushed the fix.maven.publish.workflow.2.x.1 branch from 8d69d7a to b50ce91 Compare July 12, 2024 13:20
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 13:20 — with GitHub Actions Inactive
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 13:20 — with GitHub Actions Inactive
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 13:20 — with GitHub Actions Inactive
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 13:21 — with GitHub Actions Inactive
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 13:21 — with GitHub Actions Inactive
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 13:21 — with GitHub Actions Inactive
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 14:19 — with GitHub Actions Inactive
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 14:19 — with GitHub Actions Inactive
@reta reta temporarily deployed to ml-commons-cicd-env July 12, 2024 14:19 — with GitHub Actions Inactive
@dhrubo-os dhrubo-os merged commit 150b32f into opensearch-project:2.x Jul 12, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants