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

upgrade to gradle 8 #63

Closed
wants to merge 1 commit into from
Closed

Conversation

austek
Copy link
Contributor

@austek austek commented May 19, 2024

upgrade to gradle 8
upgrade depencies

@YOU54F
Copy link
Member

YOU54F commented May 21, 2024

just pushed under a branch on origin, to see if some of the failures are reltaed to being from an outside PR

https://github.com/pact-foundation/pact-plugins/tree/austek-gradle_8

@austek
Copy link
Contributor Author

austek commented May 21, 2024

just pushed under a branch on origin, to see if some of the failures are reltaed to being from an outside PR

https://github.com/pact-foundation/pact-plugins/tree/austek-gradle_8

that build only failed because of some Github limits

"message": "You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 3858:1228:3C7CB28:3CF738A:664CAE33."

@YOU54F
Copy link
Member

YOU54F commented May 22, 2024

Yeah they get me everytime if I raise a PR which has a branch on origin, as you get two sets of jobs, and they all publish unit test results back. We should probably update that to not allow it to fail the build, if we are hitting limits.

re-triggered job and they've passed now

Thanks for the change @austek. I'll add Ron as a reviewer as well, but I shall give it an approve

@YOU54F YOU54F requested a review from rholshausen May 22, 2024 12:43
@YOU54F
Copy link
Member

YOU54F commented May 22, 2024

I think to get this merged you may want to at least b)

a) squash your commits
b) use conventional commit standards (so chore(deps): update jvm driver to gradle 8) etc as they get picked up in the changelogs. (https://github.com/pact-foundation/pact-plugins/blob/main/drivers/jvm/CHANGELOG.md)

Apart from that looks good to me, but I don't often test the consumption of the jvm-driver in pact-jvm so unsure if this may cause an issue for consumers of the jvm-driver

@YOU54F
Copy link
Member

YOU54F commented May 22, 2024

ty Ali!

@@ -26,8 +24,8 @@ subprojects {

version = '0.4.3'

targetCompatibility = '11'
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this needs to be able to run on JDK 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.

au.com.dius.pact.core:support:4.6.9 is not compatible with JDK 11

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we will have to use 4.5.9

id 'groovy'
}

version = '0.0.0'
targetCompatibility = '11'
sourceCompatibility = '11'
targetCompatibility = '17'
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be 11 here with 4.5.x branch of pact-jvm?

@austek
Copy link
Contributor Author

austek commented Jun 25, 2024

@rholshausen is this something you'd like to get merged, if not, I'll close the PR

@YOU54F
Copy link
Member

YOU54F commented Jun 25, 2024

Hmm has this already been cherry picked here in master

5a8e1df

@rholshausen
Copy link
Contributor

I thought I modified the PR and then merged. But I guess it is see it as a new commit and still labels it as unmerged.

@YOU54F
Copy link
Member

YOU54F commented Jun 25, 2024

Tres bizarre! Thanks @rholshausen and @austek 💛

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