-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[Endpoint] badge #2473
[Endpoint] badge #2473
Conversation
This reimplements the idea @bkdotcom came up with in #1519, and took a stab at in #1525. It’s a really powerful way to add all sorts of custom badges, particularly considering [tools like RunKit endpoints and Jupyter Kernel Gateway](#2259 (comment)), not to mention all the other ways cloud functions can be deployed these days. Ref #1752 #2259
|
This pull request introduces 1 alert when merging 02dd662 into d0c9da0 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
I don't really have any useful feedback on this one yet. React is wayyyy too new for me and I'm still trying to get a feel for that endpoint service. Will try to get back to this one hopefully soon once I've read up a bit |
Probably understanding the React code isn't that important here as it's just documentation. Looking at the review app would probably be sufficient. |
Okay I think I've found it now 😄 I was looking for an I was able to get it via direct uri at https://shields-staging-pr-2473.herokuapp.com/#/endpoint Can I point this thing to any endpoint that returns any JSON response? |
Yea, that's exactly the idea! 😁Or rather, a JSON response in the specified format. This is one I created for #2259: https://runkit.com/melnikow/version-from-requirements-txt (Oops, that formatting will need some work.) |
Gotcha. So this is conceptually similar to the dynamic badges, except that it offers the users the ability to define their own processing functionality |
Do we have any idea how we might define guidelines recommendations for when a user should use this type of badge vs. implementing their own service in the Shields codebase and submitting that as a PR? |
Right. It also allows a service to publish a prerendered badge which can be proxied through shields.io to provide formatting flexibility.
It's a great way to get something going quickly and to quickly iterate and remix. Beyond that I'd say if folks have something that's working well and they think it's generally useful, they should open an issue to suggest it. I don't have clear guidelines of what's too specific or too generic to include. We probably wouldn't merge "is such-and-such the current month or not" (#1519) or "how many days are left in the year," however I could see building something along those lines that's specific to hacktoberfest. This is also a good candidate for things we don't want to do, like exact star count or download count (#742), or nuget aggregate download counts (#204). |
Nice! I was thinking along the same lines of use cases. I was envisioning a scenario where someone may use this to get something going quickly, but then once they get it working well they just use it as is and not bother to share it back with the community 😄 Obviously that's not something that can be prevented, but maybe just a note in docs down the line that includes the exact sentiment you articulated:
|
Right, that's a possibility. Though the advantage of pulling the implementation into the shields server is that it will take load off their server endpoint, and probably will perform better and be more reliable. |
One odd thing I've noticed is that the first time I hit the endpoint page I see what I showed above in #2473 (comment), but after I reload the page I see this: |
lib/validate.js
Outdated
@@ -11,17 +11,21 @@ function validate( | |||
includeKeys = false, | |||
traceErrorMessage = 'Data did not match schema', | |||
traceSuccessMessage = 'Data after validation', | |||
allowAndStripUnknownKeys = true, |
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.
i have no clue what all this is used for, but my first thought here was whether any consumers of this function would ever need different values for these two keys (allowUnknown false and stripUnknown true)?
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.
It seems unsafe to allowUnknown
and not stripUnknown
so if we did that, I'd tend to think we weren't doing a good job of validating.
Originally I'd passed options
directly through which would have supported any combination, though I'm not sure we would ever want to do that.
|
||
const { protocol, hostname } = new URL(url) | ||
if (protocol !== 'https:') { | ||
throw new InvalidParameter({ prettyMessage: 'please use https' }) |
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.
👍 on pushing for https
I went through the changes and left feedback/questions where I could, but have to defer on the frontend content to someone with a better eye for that sort of thing. Bravo on another big add 🍻!
That makes sense to me. I feel like there's been a lot of issues opened lately that will be resolved by this so I definitely see value in getting the functionality out even if there's some short term formatting items. |
I'm going to add a Beta label to this feature, which will give us a chance to change the API if we decide we want to. |
schemaVersion: 1, | ||
label: 'hello', | ||
message: 'sweet world', | ||
color: 'orange', |
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.
😆 For some reason my brain read this as hello sweet orange
. wonder if i am hungry...
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, don't have much to offer on the frontend content though
Do you mean you don't have much to offer on the way it's coded, or about what it's saying to the user? |
Apologies, the former. |
Opened #2837 for the formatting issues. I'm gonna ship this!!! |
And opened #2838 for further discussions. |
This reimplements the idea @bkdotcom came up with in #1519, and took a stab at in #1525. It’s a really powerful way to add all sorts of custom badges, particularly considering tools like RunKit endpoints and Jupyter Kernel Gateway, not to mention all the other ways cloud functions can be deployed these days.
I gave this a name which did not have JSON in it, so as not to confuse it with the dynamic JSON badges.
There are a few things that need doing before this can be shipped, including supporting several of the options I included in the schema.
labelColor
for the endpoint badge #2740)link (I honestly don't know much about our link support; should we have leftLink and rightLink?)(Removed for this iteration)logoPositionAlternatively, they could be removed from the schema and added later on.
Curious for feedback! And thought I would open this up for comments before I got too far along.
Ref #1534 #2259. Closes #204 #358 #742 #999 #1519 #1752 #2625.