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 Gitlab CI badge support. #539

Merged
merged 2 commits into from Feb 5, 2017
Merged

Conversation

Susurrus
Copy link
Contributor

Resolves #537.

I tested this code by modifying app/mirage/fixtures/search.js with data from my gattii crate, which has Gitlab CI configured. None of the existing projects in this file have gitlab CI, so should I add my gattii crate information to this file? If so, is there an easy way to grab this info (maybe a REST call to the real crates.io)?

I know there are badge tests in src/tests/badge.rs, but it's all one big complicated test. Should I try to integrate a gitlab badge into that existing test or create a new one? I'm not really certain what to do here that would be best so some guidance here would be helpful.

@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 1, 2017

CCing @carols10cents Since you commented on the original issue (#537)

@carols10cents
Copy link
Member

None of the existing projects in this file have gitlab CI, so should I add my gattii crate information to this file?

It's not actually be necessary for the badge to really load-- The ember tests are just looking at the generated markup. So it should be fine to add to the badges data for the external-mixin crate fixture, which is the first row, so then you can just add another assertion in with the other ones on the same row.

I know there are badge tests in src/tests/badge.rs, but it's all one big complicated test. Should I try to integrate a gitlab badge into that existing test or create a new one? I'm not really certain what to do here that would be best so some guidance here would be helpful.

In general the Rust tests are a lot longer than I would write, personally, but so far they seem to be working out okay-- there's a lot of setup and stuff that makes longer tests make more sense than in other circumstances. So go ahead and add gitlab in the same test as the other badges!

The code looks great so far! The only thing is it looks like you took out the deny warnings in src/bin/delete-crate.rs ? When you add the tests, could you take that change out please?

Thank you so much!!!! ❤️ ❤️ ❤️

@Susurrus Susurrus force-pushed the master branch 2 times, most recently from 547f2bf to 22df891 Compare February 1, 2017 05:36
@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 1, 2017

So there's one more change I need to fix, but it's blocking on Gitlab coming back up, but I'll take care of that tomorrow.

@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 1, 2017

So Gitlab's back so everything should be good now.

One thing I should comment on is that the badge link URL points to the /pipelines page for the repo, not to the commit list for the set branch as Gitlab suggests in their Markdown badge examples. This keeps it's functionality consistent with the other badges, where they all link as directly as possible to the CI results page.

@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 2, 2017

I think we're all good with this one. I also submitted rust-lang/cargo#3632, which should round out the documentation at least in the codebase itself.

@carols10cents
Copy link
Member

Looks great!!!! Thank you!!! ❤️

@carols10cents carols10cents merged commit 5bc3409 into rust-lang:master Feb 5, 2017
@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 5, 2017

Awesome, thanks @carols10cents. I'm not familiar with the deployment process for crates.io and if it eides the same trains as Rust. So you know when this might be deployed and usable?

bors added a commit to rust-lang/cargo that referenced this pull request Feb 5, 2017
Document build badge for Gitlab CI

This doesn't make sense to merge until rust-lang/crates.io#539 is merged, but I figured I'd get it all spooled up since that PR is already ready for merging.
@carols10cents
Copy link
Member

@alexcrichton manages deployment, I'm not sure how often.

@alexcrichton
Copy link
Member

Ah yes this should be deployed now!

@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 7, 2017

@alexcrichton When is deployment done? Is it as soon as a new commit is merged in by bors? This would be helpful to document somewhere I think.

@alexcrichton
Copy link
Member

When I hit a button, and yes lots of things would be useful if they were documented!

@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 7, 2017

Well I'd be happy to help! Let me know if you think I could help here.

@chris-morgan
Copy link
Member

There’s a spelling mistake in all of this: it’s GitLab, not Gitlab. (Just like GitHub is GitHub, not Github.)

Yes, this definitely needs to be fixed.

@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 7, 2017

@chris-morgan I'll submit a new PR for this (as this has already been merged) otherwise if you want to file a new issue we can track that work there.

@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 8, 2017

@chris-morgan see rust-lang/cargo#3669 and #547.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Gitlab badges
4 participants