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 support for Bitbucket Pipelines badges (Shields.io) #1934

Merged
merged 3 commits into from
Dec 5, 2019

Conversation

delan
Copy link
Contributor

@delan delan commented Dec 4, 2019

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@@ -68,6 +68,10 @@ pub enum Badge {
branch: Option<String>,
service: Option<String>,
},
BitbucketPipelines {
repository: String,
branch: String,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve made the branch attribute required for now, because while Shields.io allows the branch to be omitted, it assumes the default branch is “master”, but Bitbucket lets you choose any branch as the default branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(strictly speaking, this is a bug that Shields.io could fix by querying the Bitbucket API for the repository’s main branch, but that’s a job for another day)

@@ -0,0 +1,6 @@
<a href="https://bitbucket.org/delan/nonymous/addon/pipelines/home#!/results/branch/{{ branch }}/page/1">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the /page/1 is mandatory (e.g. this link doesn’t work)

Copy link
Member

@carols10cents carols10cents Dec 4, 2019

Choose a reason for hiding this comment

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

This URL looks like it has your username and project in it? It should use {{ repository }}, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 79f15b4

classNames: ['badge'],
repository: alias('badge.attributes.repository'),
branch: computed('badge.attributes.branch', function() {
return encodeURIComponent(this.get('badge.attributes.branch'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the URL encoding is mandatory (e.g. this link doesn’t work)

Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

The link URL should use the repository specified in the badge configuration

@@ -0,0 +1,6 @@
<a href="https://bitbucket.org/delan/nonymous/addon/pipelines/home#!/results/branch/{{ branch }}/page/1">
Copy link
Member

@carols10cents carols10cents Dec 4, 2019

Choose a reason for hiding this comment

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

This URL looks like it has your username and project in it? It should use {{ repository }}, right?

Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

Meant to "request changes", hit the wrong button, oops!

@carols10cents
Copy link
Member

Looks great now!! Thank you so much!!!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 5, 2019

📌 Commit 79f15b4 has been approved by carols10cents

@bors
Copy link
Contributor

bors commented Dec 5, 2019

⌛ Testing commit 79f15b4 with merge 05b1399...

@bors
Copy link
Contributor

bors commented Dec 5, 2019

☀️ Test successful - checks-travis
Approved by: carols10cents
Pushing 05b1399 to master...

@bors bors merged commit 79f15b4 into rust-lang:master Dec 5, 2019
bors added a commit to rust-lang/cargo that referenced this pull request Dec 5, 2019
document support for Bitbucket Pipelines badges

- merge after rust-lang/crates.io#1934
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.

5 participants