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

Adds actual build status from docker hub as discussed in issue #241 #856

Merged
merged 7 commits into from
Apr 7, 2017

Conversation

eddiewebb
Copy link
Contributor

@eddiewebb eddiewebb commented Jan 14, 2017

Building on @jrottenberg 's contribution, but adding actual status for latest build to address discussion in issue #241

screen shot 2017-01-14 at 8 56 55 am

I debated just leaving the language to duplicate "Docker Automated Build" and let's folks choose. BUt ultimately decided to use "Docker Build Status" instead, I'd use it either way.

@jrottenberg
Copy link

Oh ! Very nice

@eddiewebb
Copy link
Contributor Author

@espadrine - anything I can do to help address the backlog of pull requests?

@paulmelnikow paulmelnikow added the service-badge New or updated service badge label Mar 27, 2017
@Bekt
Copy link

Bekt commented Apr 5, 2017

Anything holding this?

server.js Outdated
badgeData.text[1] = 'failing';
badgeData.colorscheme = 'red';
} else {
badgeData.text[1] = 'building..';
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 change this to 'building', for consistency with the appearance of other badges at https://shields.io/?

server.js Outdated
try {
var data = JSON.parse(buffer);
var latest_status = data.results[0].status;
if ( latest_status == 10 ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the spacing in this if? Most of the code looks that way today, though we don't have that linter rule turned on just yet.

server.js Outdated
badgeData.colorscheme = 'red';
} else {
badgeData.text[1] = 'building..';
badgeData.colorB = '#008bb8';
Copy link
Member

Choose a reason for hiding this comment

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

I like that color!

server.js Outdated
}
var path = user + '/' + repo;
var url = 'https://registry.hub.docker.com/v2/repositories/' + path + '/buildhistory';
var badgeData = getBadgeData('docker status', data);
Copy link
Member

Choose a reason for hiding this comment

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

'docker build' seems like a clearer label, which would be more consistent with the other CI badges.

server.js Outdated
@@ -4960,6 +4960,45 @@ cache(function(data, match, sendBadge, request) {
});
}));

// Docker Hub automated integration, latest status (passed, pending, failed)
camp.route(/^\/docker\/status\/([^\/]+)\/([^\/]+)\.(svg|png|gif|jpg|json)$/,
Copy link
Member

Choose a reason for hiding this comment

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

How about using docker/build for this instead of docker/status?

@paulmelnikow
Copy link
Member

Hi, thanks for this contribution! Would love to get this merged. Left a handful of minor comments. If you could also merge master into the PR branch, you can check that the automated tests are passing.

@paulmelnikow
Copy link
Member

@eddiewebb I just saw your note above! Would be great to have some more help with the project. 😄

Would you be interested in working on some automated tests?

@eddiewebb
Copy link
Contributor Author

I thought docker/build conflicts with the existing shield (which just indicates whether it exists, not health). But I can confirm and make the other changes.

If it does conflict, would you rather replace or use a different path?

@paulmelnikow
Copy link
Member

The current badge is at /docker/automated. You can type in "docker" at the top of https://shields.io/ to filter the listing, which is up to date with what's in server.js.

@eddiewebb
Copy link
Contributor Author

Beauty. You'll see updates shortly

@eddiewebb
Copy link
Contributor Author

Sorry, didn't get to it last night, but made the changes you suggested and merged in latest from the upstream's master branch.

@eddiewebb
Copy link
Contributor Author

eddiewebb commented Apr 6, 2017

New labels:

screen shot 2017-04-06 at 9 29 57 am

Added 404 message for invalid user or repo:

screen shot 2017-04-06 at 9 40 22 am

screen shot 2017-04-06 at 9 40 07 am

@paulmelnikow
Copy link
Member

Those error badges look a little busy, how about just 'repo not found'?

This is looking great. I'll test on my machine and then should be good to go.

@eddiewebb
Copy link
Contributor Author

new look for your review:
screen shot 2017-04-06 at 11 14 14 am

@paulmelnikow
Copy link
Member

Love it! All looks good. Last step is to test this myself, which might take a day or two.

@paulmelnikow
Copy link
Member

Cool, ran this and it looks good.

This is a documentation nit. From looking at https://hub.docker.com/r/jrottenberg/ffmpeg/builds/, it seems like "latest" has a special meaning on Docker Hub. Is that correct? If so, would it be better to avoid the word "latest" in the code and documentation here, and instead say something like "most recent"?

@eddiewebb
Copy link
Contributor Author

@paulmelnikow, it is a convention many use to Tag the most recent stable build as "LATEST".

I'll update the code and comments to be most_recent to eliminate any ambiguity.

@paulmelnikow paulmelnikow merged commit b791ffd into badges:master Apr 7, 2017
@paulmelnikow
Copy link
Member

Thanks! Appreciate your responsiveness.

@niccokunzmann
Copy link
Contributor

niccokunzmann commented Apr 7, 2017

  • Should index.html also be changed andcommitted? make website should be the command to do that.
  • When will the changes go live?
  • referring to "unbounded api calls" I can create an issue for tags. Let's discuss it there.

@paulmelnikow What do you think?

@paulmelnikow
Copy link
Member

index.html: I'd prefer we just update and commit it at deploy time. It makes PRs easier to read, avoids unnecessary merge conflicts, is fewer steps for contributors, and makes it less likely contributors will accidentally update index.html instead of try.html.

I'd like to update the deploy script to do that, feel free to open an issue if you'd like to track it.

The changes will go live when Thaddée next deploys the server. How's that for a tautology?

@niccokunzmann
Copy link
Contributor

@paulmelnikow I created two issues for discussion and checked all questions as answered. Thanks!

@nwhobart
Copy link

nwhobart commented Apr 7, 2017

@paulmelnikow @eddiewebb really been looking forward to this. thanks! even though it's been merged in is there a reason it's not showing up on the homepage?

forgive me if i'm missing something obvious.

@niccokunzmann
Copy link
Contributor

niccokunzmann commented Apr 7, 2017

@hobakill

The changes will go live when Thaddée next deploys the server.

source

It is not an automated deploy but you can create an issue for that.

@paulmelnikow
Copy link
Member

I'm glad everyone is so excited about it! I'll post a note here once it happens.

@espadrine
Copy link
Member

🚀 Deployed.

@sinedied
Copy link

sinedied commented Dec 27, 2017

=> works fine
=> not found

Same repo. Am I missing something?

@paulmelnikow
Copy link
Member

Could you open a new issue, please?

@sinedied
Copy link

Sure, will do.

@mosesliao
Copy link

what is the markdown code to implement these docker badge?

@RedSparr0w
Copy link
Member

@liaogz82:
![Docker Badge](https://img.shields.io/docker/<type>/<user>/<repo>.svg)
Replace <type>, <user>, <repo> with your own values, eg:

![Docker Stars](https://img.shields.io/docker/stars/jrottenberg/ffmpeg.svg)
![Docker Pulls](https://img.shields.io/docker/pulls/jrottenberg/ffmpeg.svg)
![Docker Automated](https://img.shields.io/docker/automated/jrottenberg/ffmpeg.svg)
![Docker Build](https://img.shields.io/docker/build/jrottenberg/ffmpeg.svg)

Docker Stars
Docker Pulls
Docker Automated
Docker Build

@mosesliao
Copy link

mosesliao commented Jun 25, 2018 via email

@eddiewebb
Copy link
Contributor Author

Please open an issue.

eddiewebb added a commit to eddiewebb/shields that referenced this pull request Nov 14, 2018
@thiagospassos
Copy link

thiagospassos commented Jan 26, 2019

For me it shows manual even though it's automated

@paulmelnikow
Copy link
Member

Hi! This PR is two years old. Could you please open a new issue?

@badges badges locked as resolved and limited conversation to collaborators Jan 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.