-
Notifications
You must be signed in to change notification settings - Fork 440
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
Add incremental build support to gradle plugin #1165
Add incremental build support to gradle plugin #1165
Conversation
@anatoliy-balakirev Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@anatoliy-balakirev Thank you for signing the Contributor License Agreement! |
So I'll admit that I've had similar issues while working through my refactor of this problem as well. Like you've seen the global mutable state either via the extension updates from within tasks or via the This plugin really needs to have a pretty major refactoring to bring it up to what would be considered current standards. You've definitely gone a pretty substantial way there. Some things that I'd like to note as pieces we should also consider:
These are just some of the things that are coming to mind currently. I'll try to get my branch that I'm working on cleaned up and pushed out for a compare and contrast if anybody is interested? |
@shanman190 oh, so you worked on the same in parallel? You sould've mentioend this in the issue, could save us quite some time... Regarding your points:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a lot of changes, so I've gone only through some of them.
Why did you delete ContractVerifierConfigProperties
? I understand that we might not want to have it in the plugins, but we can just remove it from the plugins and not touch most of the code
...b-runner/src/main/java/org/springframework/cloud/contract/stubrunner/ContractDownloader.java
Outdated
Show resolved
Hide resolved
...b-runner/src/main/java/org/springframework/cloud/contract/stubrunner/ContractDownloader.java
Outdated
Show resolved
Hide resolved
.../src/test/groovy/org/springframework/cloud/contract/stubrunner/ContractDownloaderSpec.groovy
Outdated
Show resolved
Hide resolved
.../src/main/groovy/org/springframework/cloud/contract/verifier/plugin/ContractsCopyTask.groovy
Outdated
Show resolved
Hide resolved
Yup, that's a problem.
I guess we're opinionated. We want to have the stub jar publication |
@marcingrzejszczak
So I had a couple of options to proceed with my refactoring:
However, option #2 is very fragile:
Based on this + based on the fact that some other contributors wanted to do this refactoring anyway (there are TODOs in the code) - I decided to go for option #1 as a more clear / robust solution. So I now we have following setup:
So we still need another follow up PR to remove |
I agree so we can convert the inputs and outputs separately from removing that property configuration.
I dislike being extremely explicit cause afterwards, if there's one more field needed to be used I need to refactor a milion places. Also, often, I have to deprecate an existing public constructor and create a new one. Remember that if we have a public API then we can't just remove stuff. We need to deprecate it and remove it when we have version e.g. 3.0.0 Even if we decide to go that way, I'd prefer that to be done in a separate PR.
Let's go with that if possible. If you think that option 2 makes more sense and won't interfere with the public API then let's go with option 2.
We can try to find those points and first change the process somehow and only then change the API? WDYT? |
Yes, there is always some tradeoff... It's either easier to add something without changing many places, but then harder to read / understand what we ended up having, or harder to modify, but easier to reason about each service's API. As at the end developers are spending much more time reading the code than actually writing it - I would still go for the approach with properly defined APIs, TBH. Anyway, let me try to roll back my |
Codecov Report
@@ Coverage Diff @@
## master #1165 +/- ##
========================================
Coverage ? 2.27%
Complexity ? 18
========================================
Files ? 60
Lines ? 2158
Branches ? 134
========================================
Hits ? 49
Misses ? 2101
Partials ? 8 Continue to review full report at Codecov.
|
...b-runner/src/main/java/org/springframework/cloud/contract/stubrunner/ContractDownloader.java
Show resolved
Hide resolved
...b-runner/src/main/java/org/springframework/cloud/contract/stubrunner/ContractDownloader.java
Show resolved
Hide resolved
.../groovy/org/springframework/cloud/contract/verifier/converter/RecursiveFilesConverter.groovy
Show resolved
Hide resolved
...ain/groovy/org/springframework/cloud/contract/verifier/plugin/GenerateServerTestsTask.groovy
Outdated
Show resolved
Hide resolved
@marcingrzejszczak I've reverted all my changes related to |
...b-runner/src/main/java/org/springframework/cloud/contract/stubrunner/ContractDownloader.java
Show resolved
Hide resolved
...b-runner/src/main/java/org/springframework/cloud/contract/stubrunner/ContractDownloader.java
Show resolved
Hide resolved
...b-runner/src/main/java/org/springframework/cloud/contract/stubrunner/ContractDownloader.java
Outdated
Show resolved
Hide resolved
So on this one, I've got a story for you. In the case that I have, I'd really honestly go down the path of deprecating this and forcing the end user to specifically add the stubsJar as an artifact that they publish themselves. Story time... In our environment (my workplace), we actually have to have every project disable the publications as a result of an oddity we encountered and have just had time to workaround, but not really discuss until now. Basically, our build server is only allowed to push artifacts into our Artifact repository, but not allowed to delete. In several cases, we have teams that are using SCC in the form of CDC as part of the service application Gradle module. This module publishes a Spring Boot jar, source jar, etc. In our case, the stubsJar publication actually conflicts with our normal
Now this definitely doesn't need to be a part of this pull request in order to achieve task caching, but I think that it's something to talk about. @marcingrzejszczak, I can write up an issue if you'd prefer to handle it that way or to keep it as a reminder? |
Something weird is going on with the build after I synced with original repo...
While locally it all looks good:
Moreover, reported failures are in the tests I didn't even touch. @marcingrzejszczak I see those are new tests added by you today. Any idea what's going on there? |
.../src/main/groovy/org/springframework/cloud/contract/verifier/plugin/ContractsCopyTask.groovy
Outdated
Show resolved
Hide resolved
...ovy/org/springframework/cloud/contract/verifier/plugin/GenerateClientStubsFromDslTask.groovy
Outdated
Show resolved
Hide resolved
...ain/groovy/org/springframework/cloud/contract/verifier/plugin/GenerateServerTestsTask.groovy
Outdated
Show resolved
Hide resolved
...n/groovy/org/springframework/cloud/contract/verifier/plugin/GradleContractsDownloader.groovy
Outdated
Show resolved
Hide resolved
...in/groovy/org/springframework/cloud/contract/verifier/plugin/StubRunnerOptionsFactory.groovy
Show resolved
Hide resolved
@anatoliy-balakirev, here's my work in progress if you want to use some of it for inspiration. Mine is going way left field in terms of changes, so yours will make more sense in the short term. |
@shanman190 Nice, will take a look. Thanks! |
@shanman190 hey thanks for the story :) Let's create a separate issue. Removing that default would be a breaking change :/ |
@anatoliy-balakirev eee no idea. Our builds work fine in jenkins. Are your sure you synced with the latest changes? |
@marcingrzejszczak I created another small PR to fix a couple of NPEs and it fails with the same: PR is here: #1170 Both that one and this are based on the latest master, so there is definitely some issue with it / its config for CI: Also it started failing when I rebased it yesterday. |
Can you rebase against master and try to build it again with the new base image for circleci? |
Jacoco was the culprit. Please rebase and it should work fine. |
@marcingrzejszczak that helped, thanks! |
@marcingrzejszczak @shanman190 I've updated PR with all the latest changes. Switched extension to use lazy properties as well. Take a look pls, I believe it should be the final version. |
In order to make it working - I had to define clear input and output params for each gradle task. As part of this task I've also cleaned up some services to define clear inputs instead of generic `ContractVerifierConfigProperties`. Fixes gh-1133
The last rebase was to restart CI tests as they seem to be flaky. |
Congrats @anatoliy-balakirev and @shanman190 on this fantastic PR. Great work! |
Thanks! :) |
@marcingrzejszczak I just realised that this PR introduced some kind of backward incompatible change in terms of configuring plugin in the
So when using Kotlin DSL, instead of writing this:
We'll have to do this (note
Groovy DSL is not affected. |
Yeah that's exactly what we had to do here 3c4772b It's not a breaking change though since there was no support whatsover for Kotlin. However having consistent behavior would be great. Sure, file another PR :) |
I checked this further and looks like currently it's not really possible to do, as Kotlin doesn't support properties with overloaded setters. Here is an original discussion: And a couple more related links: So for now Kotlin users will have to write That big assignment ( |
In order to make it working - I had to define clear input and output
params for each gradle task.
As part of this task I've also cleaned up some services to define clear
inputs instead of generic
ContractVerifierConfigProperties
.inputs instead of genericContractVerifierConfigProperties
.Fixes gh-1133