Skip to content
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

improve [MavenCentral], [MavenMetadata], and [GradlePluginPortal] #6628

Merged
merged 24 commits into from Jun 30, 2021

Conversation

anatawa12
Copy link
Contributor

@shields-ci
Copy link

shields-ci commented Jun 12, 2021

Messages
📖 ✨ Thanks for your contribution to Shields, @anatawa12!

Generated by 🚫 dangerJS against 1426170

@anatawa12
Copy link
Contributor Author

Question: Is there any way to test expectBadge with following redirections.

@PyvesB PyvesB added the service-badge Accepted and actionable changes, features, and bugs label Jun 13, 2021
@PyvesB
Copy link
Member

PyvesB commented Jun 13, 2021

Question: Is there any way to test expectBadge with following redirections.

I'm not sure. Generally we use expectRedirect to check that the redirect is setup correctly, and only test the main path via expectBadge.

@anatawa12
Copy link
Contributor Author

@PyvesB Thank you. So, I removed old tests for maven central.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6628 June 15, 2021 03:29 Inactive
@PyvesB
Copy link
Member

PyvesB commented Jun 19, 2021

Thanks, will take a look in the coming days! :)

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6628 June 22, 2021 07:04 Inactive
@PyvesB
Copy link
Member

PyvesB commented Jun 22, 2021

I've done a first review pass and test in the Heroku app. For anyone reading through this PR, there's been a bit of scope creep: it's not only the refactor of the redirectors mentioned in #6449 (comment), support for version suffix filters was added as well.

Looking at things in the review app, searching for "maven" displays the badges with a huge bit of vertical spacing, due to the long titles. Is anyone else observing the same problem? See screenshot below.

Screenshot 2021-06-22 194119

The only badge that currently supports versionPrefix is Maven Central. However, we've ended up in a inconsistent state with regard to the URL structures in this pull request:

  • for Maven Metadata, versionPrefix is a query parameter, whereas it's a path element in Gradle and Maven Central.
  • for Gradle and Maven Central versionPrefix is a path element whereas versionSuffix is a query parameter.

Now that we're using redirectors, it should be easy enough to resolve these inconsistencies. Switching everything to query parameters across all three badge types would make most sense to me.

@shields-cd shields-cd had a problem deploying to shields-staging-pr-6628 June 23, 2021 13:48 Failure
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6628 June 23, 2021 13:56 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6628 June 23, 2021 14:00 Inactive
@anatawa12
Copy link
Contributor Author

The only badge that currently supports versionPrefix is Maven Central. However, we've ended up in a inconsistent state with regard to the URL structures in this pull request:

  • for Maven Metadata, versionPrefix is a query parameter, whereas it's a path element in Gradle and Maven Central.
  • for Gradle and Maven Central versionPrefix is a path element whereas versionSuffix is a query parameter.

Now that we're using redirectors, it should be easy enough to resolve these inconsistencies. Switching everything to query parameters across all three badge types would make most sense to me.

I agree with that so I changed all example to use versionPrefix of queryParams. versionPrefix for gradle-plugin-portal is added by this PR so I removed support with path.

I've done a first review pass and test in the Heroku app. For anyone reading through this PR, there's been a bit of scope creep: it's not only the refactor of the redirectors mentioned in #6449 (comment), support for version suffix filters was added as well.

Looking at things in the review app, searching for "maven" displays the badges with a huge bit of vertical spacing, due to the long titles. Is anyone else observing the same problem? See screenshot below.

Screenshot 2021-06-22 194119

I found that we can generate badges without some query parameters in web UI so, To resolve this problem, I removed examples without versionPrefix or versionSuffix.

Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

Thanks, this is almost ready to go as far as I can tell!

I found that we can generate badges without some query parameters in web UI so, To resolve this problem, I removed examples without versionPrefix or versionSuffix.

Given that you've removed the examples without the filters, would it be worth being extra clear about them being optional in the remaining examples? You can add a bit of documentation in the UI, you can take a look at our Discord implemention to see how that's done:

const documentation = `

Maybe something along the following lines:

versionPrefix and versionSuffix allow narrowing down the range of versions the badge will take into account, but they are completely optional.

namedParams: {
pluginId: 'com.gradle.plugin-publish',
},
staticPreview: {
label: 'plugin portal',
message: 'v0.14.0',
label: 'maven-central',
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this have remained plugin portal?

version === undefined
)
throw new NotFound({
prettyMessage: 'version prefix or suffix not found',
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a test for this case?

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6628 June 28, 2021 10:50 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6628 June 28, 2021 10:53 Inactive
@anatawa12
Copy link
Contributor Author

@PyvesB I've fixed gradle-plugin-portal and added documentation and tests.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6628 June 28, 2021 10:59 Inactive
Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

I noticed a minor bug in our frontend, only the first query parameter has an e.g. in front of it, which looks a bit weird:
Screenshot 2021-06-30 081030

However, that's unrelated to your work, just happened to notice it whilst testing your pull request. :)

Anyway, thanks for working on this, it's good to go! 👍🏻

@PyvesB
Copy link
Member

PyvesB commented Jun 30, 2021

I was a little slow on the review, so I've tried to be fast on the deployment side: this is already live on our production servers! 🍾

@anatawa12 anatawa12 deleted the maven-central-and-metadata branch June 30, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants