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 arbitrary git ref in check breaking #48

Closed
fsommar opened this issue Apr 14, 2020 · 17 comments
Closed

Add support for arbitrary git ref in check breaking #48

fsommar opened this issue Apr 14, 2020 · 17 comments
Labels
Feature New feature or request

Comments

@fsommar
Copy link

fsommar commented Apr 14, 2020

The buf check breaking command has support for branches and tags when checking against a git repository input. I want it to accept an arbitrary git reference, like refs/remotes/origin/master or refs/pull/3/head.

In my humble opinion, if you're not afraid of breaking backwards compatibility, it makes sense to remove branch and tag flags, since they're just a strict subset of providing the reference directly (and removing them would simplify some of the flag logic).

@bufdev
Copy link
Member

bufdev commented Apr 14, 2020

We can likely do that, however we'd probably want to keep the branch and tag flags (or merge into branch) - Buf's goal is simplicity, and most people think in terms of the short branch name, not in terms of refs in our opinion.

@bufdev bufdev added the Feature New feature or request label Apr 14, 2020
@fsommar
Copy link
Author

fsommar commented Apr 14, 2020

We can likely do that, however we'd probably want to keep the branch and tag flags (or merge into branch) - Buf's goal is simplicity, and most people think in terms of the short branch name, not in terms of refs in our opinion.

That makes sense, it also follows the principle of least astonishment for most users.

@robsonmeemo
Copy link

Is there a workaround right now to compare an outstanding PR with the merge-base where it was created from? Maybe I need to create a temporary tag. :(

@robsonmeemo
Copy link

FTR, here's the workaround I'm using:

GIT_REPO=$PWD/.git
current_branch=$(git rev-parse --abbrev-ref HEAD)
master_commit=$(git merge-base master $current_branch)
git tag __orig_master_branch__ $master_commit
buf check breaking --experimental-git-clone --against-input "$GIT_REPO#tag=__orig_master_branch__"
git tag -d __orig_master_branch__

@bufdev
Copy link
Member

bufdev commented May 16, 2020

FYI investigating this now, will update this issue when we have more information. We likely will still recommend the branch/tag options, as it lets use use effectively --depth 1 --branch BRANCH_OR_TAG --single-branch for performance, but we may make it so arbitrary references are able to be checked out as well, even if it costs in terms of performance for users who choose to do that.

@bufdev
Copy link
Member

bufdev commented May 31, 2020

@fsommar @robsonmeemo is there anything preventing you from using the tar or zip formats, i.e.:

buf check breaking --against-input https://github.com/bufbuild/buf/archive/refs/pull/74/head.tar.gz#strip_components=1
buf check breaking --against-input https://github.com/bufbuild/buf/archive/refs/pull/74/head.zip#strip_components=1

Or is this not for GitHub?

@robsonmeemo
Copy link

What does 74 stand for in your example? I want to check against one specific ref, like 08fb6ab1bb1f6b1b1f1d1d6565d82f178d49c2fb

@bufdev
Copy link
Member

bufdev commented Jun 1, 2020

It's just an example. You can do this for commits as well:

buf check breaking --against-input https://github.com/bufbuild/buf/archive/43b4d4608a9e4d86a9d1826029adfe1374029a38.tar.gz#strip_components=1

Does this solve your problem? Perhaps we should update the documentation to point people to this if they want to do specific commits (the https/tarball method is also cheaper than git).

@robsonmeemo
Copy link

We do not have https access to our private repo. We use ssh credentials and I don't think the tar.gz archives are supported in that scenario.

@bufdev
Copy link
Member

bufdev commented Jun 1, 2020

OK - it should be easy to add, you just need either:

  1. .netrc credentials
  2. BUF_INPUT_HTTPS_USERNAME or BUF_INPUT_HTTPS_PASSWORD set as secrets

Secrets should be easy enough for GitHub Actions, Travis, and CircleCI https://github.com/bufbuild/buf-example

https://buf.build/docs/inputs#authentication

We can still work to add refs for git, but it's much less efficient than the tarball scanerio, I'd honestly recommend setting this up anyways.

@robsonmeemo
Copy link

We were purposely not using secrets and giving the CI app limited access to our repo as a more secure solution. We'd also like to avoid fetching from github and just comparing against the ref in the local repo.

@bufdev
Copy link
Member

bufdev commented Jun 1, 2020

OK fair enough. We'll keep this open and get a solution.

@vladdy
Copy link

vladdy commented Jun 11, 2020

I just wanted to mention that we are having a similar problem with integrating buf into my CI/CD.

@bufdev
Copy link
Member

bufdev commented Jun 11, 2020

We will get to this soon, sorry for the delay.

@vladdy
Copy link

vladdy commented Jun 11, 2020

Sure! Thank you for the prompt answer, but I just wanted to add a data point. Let me know if there is something I can help with.

@bufdev
Copy link
Member

bufdev commented Jun 16, 2020

General plan:

  • Four options: branch, tag, ref, depth.

  • The options ref and depth are new.

  • If only one of branch OR tag is specified, clone with --branch BRANCH_OR_TAG --single-branch --depth 1.

  • If branch AND ref are both specified, clone with --branch BRANCH --depth DEPTH and then git checkout REF, where depth defaults to 50.

  • If branch AND tag AND ref are all specified, error.

  • If branch OR tag, depth are specified, error - this doesn't make sense in the context of buf.

  • if ref only is specified, do clone with --depth DEPTH (defaulting to 50), this will assume user knows what they are doing

Regardless, split out submodule cloning into a separate command that comes after any potential checkout, using git submodule update --init --recursive --depth DEPTH, where depth defaults to 50.

@bufdev
Copy link
Member

bufdev commented Jun 17, 2020

This is released as v0.17.0. The documentation at https://buf.build/docs/inputs#git has been updated as well. Let us know if you have any issues.

@bufdev bufdev closed this as completed Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants