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

Remove VCSCommit and add Commit.source_control_url #55

Merged
merged 6 commits into from
Jan 9, 2024

Conversation

nicksnyder
Copy link
Member

The BSR doesn't need to be in the business of storing source control information--we don't semantically need a vcs commit hash for example and we don't care what type of source control is being used.

As a convenience for users, we DO want to store a link back to the relevant commit in source control so that a user can find relevant info (e.g. commit description, PR discussion, authors, approvers, etc.).

This change simplifies the data model to only store what we need, which is a single source control URL per commit.

@nicksnyder nicksnyder requested a review from bufdev as a code owner January 9, 2024 19:14
Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

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

I worry a bit that we're going to have a feature in the future where we do want the hash, and it doesn't cost us much to store, and it's not too expensive for us to provide it, BUT with that said I'm OK with this

@akshayjshah
Copy link
Member

I worry a bit that we're going to have a feature in the future where we do want the hash, and it doesn't cost us much to store, and it's not too expensive for us to provide it, BUT with that said I'm OK with this

I felt the same way, but I'm having a really hard time figuring out what that situation might be.

I don't think we can have the BSR call Github or git directly, because it'll open up a lot of questions about accessing files beyond the schemas. If the BSR can't access git or Github, I can't imagine anything git-specific that we'd be able to do. Multiple BSR commits can have the same VCS hash (in theory), so we decided not to allow resolving BSR commits by VCS hash.

Given all that, the only use case I can really imagine is search. I think we can enable that pretty easily by parsing the URL, breaking the path into segments, and throwing away segments less than N characters/containing non-hex characters.

On the plus side, this approach clearly gets the BSR out of the source control business: we just link to source control via a URI, and we only use it for providing context to users.

@nicksnyder nicksnyder merged commit e46c321 into main Jan 9, 2024
6 checks passed
@nicksnyder nicksnyder deleted the source-control-url branch January 9, 2024 21:54
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