-
Notifications
You must be signed in to change notification settings - Fork 499
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 #3268: Introduce Maven pinning artifacts Mechanism #3270
Fix #3268: Introduce Maven pinning artifacts Mechanism #3270
Conversation
@rt4914 PTAL. |
It looks like the |
…in_required attribute in maven_install()
@rt4914 @BenHenning Should I update the CODEOWNERS file to mention appropriate CODEOWNERS for maven_install.json? |
…ce-maven-pinning-artifacts
@rt4914 @fsharpasharp PTAL. |
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.
Looks good to me.
Unassigning @fsharpasharp since they have already approved the PR. |
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.
@prayutsu Update the code-owner file to mention Ben's name for maven file. Also assigning this to Ben.
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.
@prayutsu apologies for the delayed review. PTAL at my latest comments--I think you might've made a mistake when changing WORKSPACE to point to third_party/maven_install.json.
Also, just to check: doesn't maven_install.json need to be moved to third_party/?
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.
@prayutsu I think you need to add a CODEOWNERS entry for third_party/ (that can go to me). Also, it looks like the maven_install.json file needs to be regenerated.
Beyond that, the Gradle test failures seem to be #2844 which is disappointing--I thought that was addressed. Rerunning them should hopefully fix the issue.
Done. |
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 @prayutsu. Just two last quick comments.
Per previous runs, the compute affected tests script doesn't seem to produce any tests to run here, so skipping it since the last two runs have hung. |
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 @prayutsu. This LGTM!
Unassigning @BenHenning since they have already approved the PR. |
Explanation
Fixes #3268:
Introduced Pinning Artifacts mechanism for Maven dependencies.
Checklist