-
Notifications
You must be signed in to change notification settings - Fork 77
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
Fix download snapshot warnings #882
Fix download snapshot warnings #882
Conversation
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.
Great start! Thanks for the various improvements throughout and the cache introduction to fix the underlying issue. Let me know once you've worked through the final todo before a last review & merge.
src/main/java/org/openrewrite/maven/MavenMojoProjectParser.java
Outdated
Show resolved
Hide resolved
5808c75
to
cedabc5
Compare
cedabc5
to
547e934
Compare
src/main/java/org/openrewrite/maven/MavenMojoProjectParser.java
Outdated
Show resolved
Hide resolved
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.
Great to see this @ammachado ; it was a bit of a puzzle for me correlating this with the MavenParser in openrewrite/rewrite, but I came up with a couple of questions that would be good to address I think. Curious to hear your thoughts, as you've perhaps already looked into those aspects.
I've also added a minimal test since I noticed none were using snapshot versions before. We might want to extend that with some assertions, although that might be a bit of a challenge with the testing framework used for plugins.
project.getGroupId(), | ||
project.getArtifactId(), | ||
project.getVersion(), | ||
project.getVersion().endsWith("-SNAPSHOT") ? null : project.getVersion() |
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.
Looking at MavenPomDownloader I wonder if we should even include a dated snapshot version here, or always have that be null
or project.version()
. What made you introduce this ternary?
src/main/java/org/openrewrite/maven/MavenMojoProjectParser.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Tim te Beek <timtebeek@gmail.com>
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.
Thanks again @ammachado ! Great to see these cleared out.
What's changed?
Fix download warnings for SNAPSHOT components
What's your motivation?
Anything in particular you'd like reviewers to focus on?
No
Anyone you would like to review specifically?
@timtebeek
Have you considered any alternatives or workarounds?
No
Any additional context
No.
Checklist