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

Add [PingPong] Service #6327

Merged
merged 14 commits into from
May 8, 2021
Merged

Add [PingPong] Service #6327

merged 14 commits into from
May 8, 2021

Conversation

bluecap-se
Copy link
Contributor

Adds two new badges for PingPong Status Page service:

  • Uptime (last 30 days)
    Uptime
  • Status
    Uptime

@shields-ci
Copy link

shields-ci commented Mar 27, 2021

Messages
📖 ✨ Thanks for your contribution to Shields, @bluecap-se!

Generated by 🚫 dangerJS against 1aa731f

@lgtm-com
Copy link

lgtm-com bot commented Mar 27, 2021

This pull request introduces 2 alerts when merging 4515614 into 27951e9 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@calebcartwright calebcartwright added the service-badge New or updated service badge label Mar 28, 2021
@calebcartwright
Copy link
Member

hi, thanks for the PR! Could you take a look and address the failing CI checks?

They seem to be linter related, and you should be able to reproduce locally via npm run lint

@bluecap-se
Copy link
Contributor Author

Thanks for your feedback @calebcartwright! Please have another look after the fixes.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6327 April 1, 2021 23:52 Inactive
@calebcartwright
Copy link
Member

Can you tell us a little more about the API token? I assume that it's absolutely necessary in order to fetch the data?

Can this token have it's permissions/auth. scoped to a read-only mode, or does it inherently provide other permissions as well?

@bluecap-se
Copy link
Contributor Author

Of course. The API token is a public read-only token that's specific for status/uptime badges, making it safe for public sharing. It's required to the fetch the data, otherwise the backend returns 404. It has no other permissions and is separate from the API key with full permissions that's used for the REST API.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and additional info! It seems reasonable to incorporate these badges, though I think we've got an opportunity to simplify the implementation somewhat by reusing some standard Shields helpers.

Have left specific details inline

Comment on lines 42 to 48
static render({ message, color }) {
return {
label: 'status',
message,
color,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The approach taken here is basically just using us as a passthrough for whatever values the upstream provider happens to set, which isn't how we utilize values from the upstream endpoints.

A core goal for the Shields project is to provide consistency and stay aligned to our badge specification, and that's not really something we can be sure we're adhering to if we let an upstream service specify core badge attributes like the message and color.

Instead, what we do is inspect the value(s) and only the value(s) from the response we get from the upstream service in order to figure out what we should set attributes like the message and color of our badges to be. In many cases for common categories of badges we also have utility/helper functions that handle common concerns, like rendering and query params, which helps us both ensure consistency across our badge offerings and simplifies the codebase.

I think there's a great opportunity to do that here, and I'd suggest taking a look at some of the service classes for our other badges in the monitoring category, such as nodeping, as a frame of reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good points. In the update, I've referenced uptimerobot as it seemed to match this use case; nodeping example doesn't have all status types (issues, maintenance being the case here) but it pointed me in the right direction - thanks for that! If there's a helper function in Shields that supports all status types, I'd be happy to apply it here.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thank you for the changes and apologies for the extended delay on the second review; this one fell off the radar!

This is in much better state now, though with a few comparatively minor things that'll need to be adjusted before merging

@calebcartwright
Copy link
Member

Updates look good thanks! Could you rebase on master? We have the up-to-date branch gate enabled so will need to get this in sync with master before it can be merged, but should be good to go afterwards

@PyvesB
Copy link
Member

PyvesB commented May 5, 2021

Great to see this work completed, thanks @bluecap-se and @calebcartwright for your work! :)

@calebcartwright
Copy link
Member

Oops, looks like another PR got merged inbetween and this is out of date again. Will require another rebase on your end sorry. IIRC you should also be able to check the box to allow maintainers to update the PR which would allow us to sync the branches for you and make it a bit easier to merge

@bluecap-se
Copy link
Contributor Author

@calebcartwright The branch should be updated again. The option to allow maintainers doesn't seem to exist on this PR.

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.

Strange that the option doesn't exist on this PR. Let's get this merged before it's too late again! :)

@PyvesB PyvesB merged commit 8794343 into badges:master May 8, 2021
@PyvesB
Copy link
Member

PyvesB commented May 8, 2021

These badges are now live! 🍾

For example: https://img.shields.io/pingpong/status/sp_2e80bc00b6054faeb2b87e2464be337e

If you've got a Twitter handle @bluecap-se , I can post a little something.

@bluecap-se
Copy link
Contributor Author

@calebcartwright @PyvesB Great to see this live, thanks for all your feedback and assistance! 🚀

Our Twitter handle is @pingpong_status

As the creator of PingPong status pages I'd like to present our offering to all open source projects: free Pro plan. If this sounds interesting for Shields, start by creating a Pro account and then ping us to activate the open source offer.

@PyvesB
Copy link
Member

PyvesB commented May 11, 2021

There you go, I sent a little something: https://twitter.com/Shields_io/status/1392012310940692482

Thanks for making us aware of the offering for open-source projects. For now I think we're good on that side, we already have an integration with one of your competitors, UptimeRobot - though there's a config problem at the moment that I'm working on fixing! :)

@bluecap-se
Copy link
Contributor Author

Sorry to see https://status.shields.io still being down, let us at PingPong know if we can assist in any way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants