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

[FLINK-34419][docker] Add tests for JDK 17 & 21 #182

Merged
merged 3 commits into from
Mar 27, 2024

Conversation

morazow
Copy link
Contributor

@morazow morazow commented Feb 28, 2024

[FLINK-34419]: Updated tests to use JDK 17 and 21 versions in dev-master

Copy link
Contributor

@XComp XComp left a comment

Choose a reason for hiding this comment

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

Switching back to not-approved to discuss the matrix approach of your first commit. :)

@morazow
Copy link
Contributor Author

morazow commented Feb 28, 2024

Thanks @XComp, I'll ping you again once I update it

@morazow morazow force-pushed the mor-FLINK-34419-dev-master branch from 2b537ed to dbebc4d Compare February 29, 2024 08:06
@morazow
Copy link
Contributor Author

morazow commented Feb 29, 2024

Morning @XComp,

I have updated the PR accordingly. I had to set the max-parallel job to 1, otherwise the occasionally the docker container names (jobmanager or taskmanager) would clash and fail to start.

I could change the test scripts to name containers so they don't clash. Please let me know.

fail-fast: false
max-parallel: 1
matrix:
java_version: [ 8, 11, 17, 21 ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you separate the commit:

  • [hotfix][ci] Updates actions
  • [hotfix][ci] Utilize matrix strategy in GHA workflow
  • [FLINK-34419][docker] Adds JDK17 and JDK21 support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! I also changed accordingly other branches

@morazow morazow force-pushed the mor-FLINK-34419-dev-master branch from dbebc4d to d760ccd Compare February 29, 2024 11:23
@morazow
Copy link
Contributor Author

morazow commented Feb 29, 2024

Another off-topic point, the tests build and package a job using mvn package command. This step pulls many transitive dependencies. Should we add GitHub action to cache the mvn jars in this issue? Or should we address it separately?

@morazow morazow force-pushed the mor-FLINK-34419-dev-master branch from 1f08b25 to 3b9dc80 Compare March 24, 2024 13:45
@morazow
Copy link
Contributor Author

morazow commented Mar 24, 2024

@XComp please have a look again. If this looks good, I will backport to other branches accordingly, thanks 🙏

Copy link
Contributor

@XComp XComp left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍 Thanks. Can you please merge the parallelism hotfix commit into matrix hotfix commit? There's no reason to keep these two separate.

@morazow morazow force-pushed the mor-FLINK-34419-dev-master branch from 3b9dc80 to cc33e95 Compare March 26, 2024 20:19
@morazow
Copy link
Contributor Author

morazow commented Mar 26, 2024

Thanks @XComp for looking into 🙏 I have squashed it as you suggested, fixed.

I will update the other PRs similar to this one tomorrow. Thanks a lot!

@XComp
Copy link
Contributor

XComp commented Mar 27, 2024

Ok, ping me when you're done updating PR #183 and PR #184

@morazow
Copy link
Contributor Author

morazow commented Mar 27, 2024

Thanks @XComp, I have updated both pull requests

@XComp XComp merged commit c350c61 into apache:dev-master Mar 27, 2024
4 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.

2 participants