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

[BitbucketLastCommit] Add Bitbucket last commit #10043

Merged
merged 9 commits into from
Mar 25, 2024

Conversation

ReubenFrankel
Copy link
Contributor

@ReubenFrankel ReubenFrankel changed the title [BitbuckerLastCommit] Add Bitbucket last commit [BitbucketLastCommit] Add Bitbucket last commit Mar 23, 2024
@ReubenFrankel ReubenFrankel reopened this Mar 23, 2024
Copy link
Contributor

github-actions bot commented Mar 23, 2024

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

Generated by 🚫 dangerJS against 610b149

@ReubenFrankel
Copy link
Contributor Author

ReubenFrankel commented Mar 23, 2024

It would be possible split this current implementation into separate branch and non-branch variations (like the GitHub and Gitea last commit API), but as far as I can see this would require an preliminary request to /repositories/{user}/{repo} to resolve the main branch:

GET https://bitbucket.org/api/2.0/repositories/shields-io/test-repo?fields=mainbranch

{
    "mainbranch": {
        "name": "main",
        "type": "branch"
    }
}

Happy to implement, but let me know what you think.

Copy link
Contributor

🚀 Updated review app: https://pr-10043-badges-shields.fly.dev

@chris48s chris48s added the service-badge Accepted and actionable changes, features, and bugs label Mar 24, 2024
@chris48s
Copy link
Member

It would be possible split this current implementation into separate branch and non-branch variations (like the GitHub and Gitea last commit API), but as far as I can see this would require an preliminary request to /repositories/{user}/{repo} to resolve the main branch:

Given BitBucket doesn't give us a way to get the default branch without making an extra request, I think it is fine to just say branch is a required param. That is what we have done for the BitBucket Pipelines badge.
There's a bit of background on this in #5318 and the linked issues

}).required()

const queryParamSchema = Joi.object({
path: Joi.string().uri({ relativeOnly: true }),
Copy link
Member

Choose a reason for hiding this comment

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

once we merge the other related PR, this could become relativeUri, 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.

Yep!

Copy link
Member

Choose a reason for hiding this comment

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

OK. I've just put #10041 in the merge queue. Once #10041 is in, do you want to update this. Then I'll give it a final check over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

services/bitbucket/bitbucket-last-commit.service.js Outdated Show resolved Hide resolved
},
},
schema,
httpErrors: { 403: 'private repo' },
Copy link
Member

@chris48s chris48s Mar 24, 2024

Choose a reason for hiding this comment

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

could we give a custom 404 error here as well to explain what wasn't found in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const data = await this.fetch({ user, repo, branch, path })
const [commit] = data.values

if (!commit) throw new NotFound()
Copy link
Member

Choose a reason for hiding this comment

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

..and then also here. Probably 'no commits found' for this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

🚀 Updated review app: https://pr-10043-badges-shields.fly.dev

@chris48s
Copy link
Member

nice - thanks 👍

@chris48s chris48s added this pull request to the merge queue Mar 25, 2024
Merged via the queue into badges:master with commit 5405dad Mar 25, 2024
21 checks passed
@ReubenFrankel ReubenFrankel deleted the bitbucket-last-commit branch March 25, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants