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.

Sign up for GitHub

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

Jump to bottom

fix: [node npm] service has bad colors #4809 #4810

Merged
merged 17 commits into from
Apr 5, 2020
Merged

fix: [node npm] service has bad colors #4809 #4810

merged 17 commits into from
Apr 5, 2020

Conversation

regevbr
Copy link
Contributor

@regevbr regevbr commented Mar 22, 2020

Fix #4809

@shields-ci
Copy link

shields-ci commented Mar 22, 2020 •
edited
Loading

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

Generated by 🚫 dangerJS against 80afde5

@regevbr
Copy link
Contributor Author

regevbr commented Mar 22, 2020

Instead of getting the current version (which can be an odd version which is unstable and will never be LTS), I get all the current LTS versions and make sure that the package matches to all of them

@regevbr
Copy link
Contributor Author

regevbr commented Mar 22, 2020

The dockercloud tests failed for some reason and the link is broken so I can't check why...

@calebcartwright calebcartwright changed the title fix: node service has bad colors #4809 fix: [node] service has bad colors #4809 Mar 23, 2020
@calebcartwright calebcartwright added the service-badge New or updated service badge label Mar 23, 2020
@shields-cd shields-cd temporarily deployed to shields-staging-pr-4810 March 25, 2020 03:03 Inactive
@calebcartwright
Copy link
Member

The dockercloud tests failed for some reason

Don't worry about that one, it's unrelated #4808

@calebcartwright
Copy link
Member

The failing service test is due to changes in this PR though. Can you please take a look whenever you get a chance @regevbr

https://circleci.com/gh/badges/shields/94808?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

NodeVersion [live] gets the node version of passport [ GET /passport.json ] - [ GET /passport.json ]
  AssertionError: expected [Function] to not throw an error but 'TypeError: Invalid comparator: intermediate' was thrown

@regevbr
Copy link
Contributor Author

regevbr commented Mar 25, 2020

@calebcartwright this is actually doesn't seem related to me - the message the live tests return is "credentials are misconfigured" and it is also mentioned in the link you sent me.
I'm trying to run it locally and it fails as well, but I'm not sure what those credentials should be... Can you please help?

@regevbr
Copy link
Contributor Author

regevbr commented Mar 25, 2020

@calebcartwright I managed to find the reason for this failure, and it is not due to my changes - one or both of NPM_ORIGINS and NPM_TOKEN env variables are missing in the ci process...
After setting those in my dev env, the tests are passing.

@calebcartwright
Copy link
Member

Yeah, seems like the node failure was spurious. Thanks for checking!

Will take a closer look at this PR over the weekend

@regevbr
Copy link
Contributor Author

regevbr commented Mar 30, 2020

@calebcartwright I have implemented a new badge.
Tested in locally and it works great.
Can you please CR?

@regevbr
Copy link
Contributor Author

regevbr commented Mar 30, 2020

seems like tests are failing due to issues with github integration and not related to my code

@calebcartwright calebcartwright mentioned this pull request Mar 31, 2020
Closed
@shields-cd shields-cd temporarily deployed to shields-staging-pr-4810 March 31, 2020 00:35 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-4810 March 31, 2020 09:07 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-4810 March 31, 2020 11:51 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-4810 March 31, 2020 11:55 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-4810 March 31, 2020 12:07 Inactive
@regevbr
Copy link
Contributor Author

regevbr commented Apr 2, 2020 •
edited
Loading

Please note I also changed the label to be node-lts as it will look better when using with tagged versions, e.g node-lts@next vs node lts@next

@calebcartwright
Copy link
Member

To sum up, I believe my fix is legit and will not harm any other services.

So the other npm badges are still using the /latest endpoint right? if so then i'm good, just want to ensure this change wasn't going to cause the all of those other npm badges to use the other endpoint and larger response size

@regevbr
Copy link
Contributor Author

regevbr commented Apr 2, 2020

They will not be affected

@regevbr
Copy link
Contributor Author

regevbr commented Apr 2, 2020 •
edited
Loading

@calebcartwright sorry for the short answer previously.
The other packages will not be affected as they are not passing tag at all those the condition I added will not be invoked. They will use /latest when the package being checked is not scoped, otherwise they will use the extended endpoint, but that is the current behavior in any case.

Even if in the future they will support tag, they will use the extended endpoint, but that is what they should use, otherwise they will get false data.

@calebcartwright
Copy link
Member

@calebcartwright sorry for the short answer previously.

No worries! You answered my question anyway. Will gives this a final lookover later today

@shields-cd shields-cd temporarily deployed to shields-staging-pr-4810 April 3, 2020 09:49 Inactive
@regevbr
Copy link
Contributor Author

regevbr commented Apr 3, 2020

@calebcartwright thanks for your review, I have revised the code accordingly

@shields-cd shields-cd temporarily deployed to shields-staging-pr-4810 April 5, 2020 01:36 Inactive
calebcartwright
calebcartwright reviewed Apr 5, 2020
View reviewed changes
services/node/node-current.tester.js
.intercept(mockCurrentSha(13))
.expectBadge({ label: 'node', message: `12`, color: `yellow` })

t.create("gets the tagged release's node version version of ionic")
Copy link
Member

@calebcartwright calebcartwright Apr 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm a little surprised that prettier didn't flag the double quotes here (and below) 🤔

Nevermind I see why now

calebcartwright
calebcartwright approved these changes Apr 5, 2020
View reviewed changes
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!

@calebcartwright calebcartwright merged commit 15cbbe8 into badges:master Apr 5, 2020
@shields-deployment
Copy link

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

@regevbr
Copy link
Contributor Author

regevbr commented Apr 5, 2020

Thank @calebcartwright for the patience and thorough review!

@regevbr
Copy link
Contributor Author

regevbr commented Apr 5, 2020

We completely forgot to add text documentations to the badges. I will create a new pr

@regevbr regevbr mentioned this pull request Apr 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calebcartwright calebcartwright calebcartwright approved these changes

Assignees
No one assigned
Labels
service-badge New or updated service badge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

badge request: Node LTS supported engines badge
4 participants
@regevbr @shields-ci @calebcartwright @shields-cd