-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(gradle-lite): JS-based Gradle manager #7521
Conversation
Ready for test runs, more thorough testing is required currently |
Challenge: could we rewrite this to be like a normal manager that takes one file instead of all files? so e.g. |
Technically, it's possible at the cost of missing upgrades for external variables. But later, I think these upgrades can be handled too, if (1) we provide some way to sort |
Actually, I tried to rely on autoreplace feature here, but then realized it won't work without per-file extraction. |
Maybe we need to allow to return an additional optional target file string to support this. If I remember correctly we could use that in gitlab-include manager too. |
lib/manager/gradle-lite/parser.ts
Outdated
fileReplacePosition: variable.fileReplacePosition, | ||
packageFile: variable.packageFile, | ||
}; | ||
dep.groupName = variable.key; |
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.
Ref: #6474
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.
The potential problem here: same variable names in different files.
Possible solution: use packageFile
in group name.
export function isDependencyString(input: string): boolean { | ||
const split = input?.split(':'); | ||
if (split?.length !== 3) { | ||
return false; | ||
} | ||
const [groupId, artifactId, versionPart] = split; | ||
return ( | ||
groupId && | ||
artifactId && | ||
versionPart && | ||
artifactRegex.test(groupId) && | ||
artifactRegex.test(artifactId) && | ||
versionPart === versionLikeSubstring(versionPart) | ||
); | ||
} |
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.
This doesn't match the alternative form of group: "com.example", name: "my.dependency", version: "1.2.3"
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.
group: "com.example", name: "my.dependency", version: "1.2.3"
Can these named parameters go in any order?
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.
yes, i think so. If i remember correctly it's like a hashtable, so order doesn't matter
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.
Well, I think it's out of scope for now
Let's aim to merge this disabled by default, so it's lowest risk. Then hopefully we get some feedback and can decide how to handle the two managers simultaneously. |
yeah, I can enable it on my gradle repo's to validate, currently only one major update awaiting approval. But have soma basic test repos on github too |
Another alternative: let |
Two points that potentially may complicate "wrapping" solution:
|
We can't update transitive dependencies, so how does the logic flow work today? e.g. do we still do a lookup and then attempt to update them but fail to update? Or is there a way that the existing manager filters them prior to update?
So we should see some updates working correctly now rather than before, assuming the new manager detects them? |
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.
Just some suggestions, otherwise LGTM. I've not checked every line, as this is very big pr. Best is to merge it as disabled / beta feature. I'm happy to test it on my gradle repo to see it in action. 🙃
lib/manager/gradle-lite/readme.md
Outdated
@@ -0,0 +1 @@ | |||
This is an alternate manager for Gradle, written completely in JavaScript. |
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.
We should add some more description, so users are welcome to disable default gradle
and test this new manager, as we plan to replace to old in future.
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.
My additions are not ideal, but at least someone can correct me instead of writing from scratch.
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.
Maybe @HonkingGoose has some time to review this 🙃
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
@zharinov what's the expected result when both gradle managers are enabled at the same time? e.g is there a predictable order of which manager updates first? Is it handled gracefully? Are the updates de-duplicated in the PR description? |
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.
LGTM to me
lib/manager/gradle-lite/readme.md
Outdated
@@ -0,0 +1 @@ | |||
This is an alternate manager for Gradle, written completely in JavaScript. |
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.
Maybe @HonkingGoose has some time to review this 🙃
I guess, it's a good time to release it. From this point, all bugs can be handled as separate issues. |
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.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.
Time to merge this 🙃
I'm very interested to see how this goes! 🎉 |
🎉 This PR is included in version 23.97.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Closes #3608, #7300, #7112