Skip to content

Conversation

StefMa
Copy link
Contributor

@StefMa StefMa commented Mar 18, 2019

Context

This is a "follow up" of #8636
I think @ljacomet has just overseen the gradlePluginPortal 😅.

I hope it is ok and break nothing to change the PluginPortal from a ArtifactRepository to an MavenArtifactRepository. But since it is an "up cast" nothing should break ✊.

I also have added the @Incubating because the gradlePluginPortal() have this as well...

Changes in the accepted-public-api-changes.json (like here) and in the "changelog" (here) should be done afterwards as well 👍

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

Copy link
Member

@ljacomet ljacomet left a comment

Choose a reason for hiding this comment

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

Please have a look at the comments.

Aside from that, and unless someone else thinks this is a bad idea, it could go in rather quickly.

@StefMa StefMa force-pushed the patch-6 branch 2 times, most recently from c5aa9ec to 73dd42a Compare March 18, 2019 14:40
@StefMa
Copy link
Contributor Author

StefMa commented Mar 18, 2019

@ljacomet fixed and CI is 🍏 🤘

@ljacomet
Copy link
Member

@StefMa Would you consider squashing your commits? Not sure the history is useful given the scope of the change. Wdyt?

Then I would trigger a full build on our CI infra to make sure all is good.

@StefMa
Copy link
Contributor Author

StefMa commented Mar 18, 2019 via email

@ljacomet
Copy link
Member

That's true, though I have no idea how it plays with our DCO check 😉

@ljacomet
Copy link
Member

Let me trigger a full build now.

@StefMa
Copy link
Contributor Author

StefMa commented Mar 19, 2019

Rebased and squashed everything ✅

This https://builds.gradle.org/viewLog.html?buildId=20995831&buildTypeId=Gradle_Check_Stage_ReadyforMerge_Trigger was failing on your CI. The other tests passed. But for whatever reasons I can't access it currently...
Maybe you can check that out?

@big-guy big-guy added this to the 5.4 RC1 milestone Mar 21, 2019
@StefMa
Copy link
Contributor Author

StefMa commented Mar 25, 2019

How I can fix this issues? 🤔

@ljacomet
Copy link
Member

My bad, forgot to leave a comment last week. The branch point you took was bad in the sense that it had a performance regression. It has been fixed since then.
A rebase of the branch on current master is your best course of action.

Thanks for your patience.

@StefMa
Copy link
Contributor Author

StefMa commented Mar 25, 2019 via email

@ljacomet ljacomet merged commit 648173b into gradle:master Mar 25, 2019
@ljacomet
Copy link
Member

And merged, thanks for the contribution

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