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

Allow specifying tag parameter for setting a custom tag in BSR #36

Closed
wants to merge 1 commit into from

Conversation

patrickjmcd
Copy link

Many users (including myself) would benefit from pushing semantic-versioned tags to the Buf Schema Registry for better versioning (for example, in a generated SDK).

This would resolve #20

@CLAassistant
Copy link

CLAassistant commented Nov 29, 2023

CLA assistant check
All committers have signed the CLA.

@arxeiss arxeiss mentioned this pull request Jan 2, 2024
Copy link
Member

@doriable doriable left a comment

Choose a reason for hiding this comment

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

Overall, I think this change is fairly reasonably. It does not support chaining multiple tags, but I think this solves the case for users where they only need to support a single tag, and so it can help unblock many folks already.

Thank you for opening up this change, I left some comments and nits!

| `buf_token` | The [Buf authentication token][buf-token] used for private [Buf inputs][input] | ✅ | [`${{github.token}}`][github-token] |
| `input` | The path of the [input] you want to push to BSR as a module | | `.` |
| `draft` | Indicates if the workflows should push to the BSR as a [draft][buf-draft] | | |
| `create_visibility` | The visibility to create the BSR repository with, if it does not already exist | | |
| `tag` | The custom tag to push to the BSR | | |
Copy link
Member

Choose a reason for hiding this comment

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

Currently, we don't support "moving"/"re-writing" the same tag to a different commit. So if a user tries to set a single, static tag, their subsequent pushes will fail. This should be called out here.

Suggestion on this documentation:

" A custom tag to push with each commit. The BSR does not currently support moving or duplicated tags, so use this with care."

Comment on lines +40 to +43
VERSION=${GITHUB_SHA}
if [ -n "${TAG}" ]; then
VERSION="${TAG}"
fi
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should have a default behaviour if no tag is set. It should either be "no tag" or "tag with configured value".

@@ -129,6 +130,26 @@ is not supported by `buf-push-action` on its own - you'll need to stitch this fu
your workflow on your own. For more details, see [this](https://github.com/bufbuild/buf/issues/838)
discussion.

### Push with a custom tag
Copy link
Member

Choose a reason for hiding this comment

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

Currently, you cannot buf push with both a --draft and a --tag. This should be clearly called out in the documentation.

Suggestion: at the end of this section, I would add the following warning:

Important note: you cannot set both `--tag` and `--draft`. You can only use one or the other.

Comment on lines +45 to 46
BUF_ARGS=("--tag" "$VERSION")
if [ "${DRAFT}" == "true" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

We should have a check that does not set both DRAFT and TAG.

@nicksnyder
Copy link
Member

Just FYI, going to hold off on merging a change like this. I posted an update on the linked issue here: #20 (comment)

@patrickjmcd
Copy link
Author

Ok, thanks @nicksnyder. Should I close this PR in favor of the upcoming changes?

@nicksnyder
Copy link
Member

Yeah, you can close the PR

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.

add --tag option
4 participants