feat: add go_version_file support #155

stevenh · 2023-08-15T00:56:20Z

Add support for determining the version of buf to use from go.mod including full test for the functionality.

This allows go projects to have a single source for go module versions.

Also:

Fixes from lint issues in README.md.

Fixes #155

CLAassistant · 2023-08-15T00:56:24Z

All committers have signed the CLA.

Add support for determining the version of buf to use from go.mod including full test for the functionality. This allows go projects to have a single source for go module versions. Also: * Fixes from lint issues in README.md. Fixes bufbuild#155

bufdev

Thanks for the PR. A few notes:

We can't promise we will merge this, but will consider it as a feature. We don't recommend adding github.com/bufbuild/buf as a way to install the CLI - this installation method is not officially supported.
I've added a few comments to start with some questions.
If we do accept this as a feature, revert all changes not related to the feature (ie changes to makefile, eslint, readme lint, etc), so that the PR only relates to the feature.

bufdev · 2023-08-15T03:16:54Z

src/run.ts

+  if (versionFilePath !== "") {
+    if (version !== "") {
+      core.warning(
+        "Both version and go_version_file inputs are specified, go_version_file will be preferred"


We would definitely prefer the version in this case

Unfortunately that's not possible as version has a default.

bufdev · 2023-08-15T03:18:10Z

src/run.ts

+  const basename = path.basename(versionFilePath);
+  if (basename === "go.mod" || basename === "go.work") {
+    const match = contents.match(
+      /^\s+github.com\/bufbuild\/buf v(\d+(?:\.\d+)*)/m


Does this work with non-released versions?

Likely not I fix and will add a test for that.

When I originally read this I was thinking that getBuf dealt with hashed versions, but it seems to rely on tagged versions so if the user had pinned a specific unreleased hash this wouldn't work.

I don't think this is an issue it's unlikely to be something that's needed, thoughts?

bufdev · 2023-08-15T03:18:36Z

action.yml

@@ -22,6 +22,9 @@ inputs:
    description: The domain of the Buf Schema Registry to login to.
    required: false
    default: 'buf.build'
+  go_version_file:
+    description: The go.mod file to read the buf version from.


When would this ever be anything except go.mod? Why is this an option?

Depending on the structure of the repo it may not be in the root.

It also supports just a raw version in a file if the basename is not go.mod, in the same way setup-go does.

bufdev · 2023-08-15T03:18:47Z

action.yml

@@ -22,6 +22,9 @@ inputs:
    description: The domain of the Buf Schema Registry to login to.
    required: false
    default: 'buf.build'
+  go_version_file:


Not sure about naming here.

This matches the name that action/setup-go does with underscores instead of dashes to follow your existing naming convention.

If you prefer just version_file that would work too?

bufdev · 2023-08-15T03:19:15Z

Makefile

@@ -30,8 +30,12 @@ format: node_modules
 lint: node_modules
 	npm run lint

+.PHONY: test


Revert changes to Makefile

Are you sure as if I did that so user wouldn't see test failures as added to support this feature?

Here's an example of the output from make test or make which now also adds test to ensure the user fixes any issues they might introduce:

PASS __tests__/buf-setup.test.ts buf-setup go-version-file ✓ return version if go_version_file isn't set (122 ms) ✓ parses go.mod format if go_version_file contains go.mod basename (106 ms) ✓ returns contents if go_version_file is set to non go.mod filename (107 ms) ✓ reports failure if go_version_file doesn't exist (2 ms) ------------|---------|----------|---------|---------|----------------------------------------- File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s ------------|---------|----------|---------|---------|----------------------------------------- All files | 48.96 | 26.82 | 62.5 | 48.96 | buf.ts | 12.85 | 0 | 0 | 12.85 | 31-186 error.ts | 100 | 100 | 100 | 100 | run.ts | 75 | 33.33 | 100 | 75 | 28,39,48,63,69,75,87-96,100-103,107-110 version.ts | 100 | 85.71 | 100 | 100 | 78 ------------|---------|----------|---------|---------|----------------------------------------- Test Suites: 1 passed, 1 total Tests: 4 passed, 4 total Snapshots: 0 total Time: 1.693 s, estimated 3 s Ran all test suites.

bufdev · 2023-08-15T03:19:30Z

README.md

@@ -1,7 +1,7 @@
 # `buf-setup-action`

-This [Action] installs the [`buf`][buf-cli] CLI in your GitHub Actions pipelines so that it can be


Revert readme lint edits

Would you like these in a separate PR?

bufdev · 2023-08-15T03:19:47Z

.eslintrc.js

@@ -12,14 +12,20 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-const ignoreFiles = [".eslintrc.js", "dist/**/*"];


Revert all changes

These are needed otherwise linting will fail.

Add support for determining the version of buf to use from go.mod including full test for the functionality. This allows go projects to have a single source for go module versions. Also: * Fix lint issues in README.md. Fixes bufbuild#155

stevenh · 2023-08-15T10:38:05Z

Thanks for the PR. A few notes:

We can't promise we will merge this, but will consider it as a feature. We don't recommend adding github.com/bufbuild/buf as a way to install the CLI - this installation method is not officially supported.

I've added a few comments to start with some questions.

If we do accept this as a feature, revert all changes not related to the feature (ie changes to makefile, eslint, readme lint, etc), so that the PR only relates to the feature.

Thanks for the review, wasn't expecting that as I left it in draft to finish off this morning ;-)

I've responded to all your notes with few questions and some clarification of the choices, looking forward to your feedback.

Add support for determining the version of buf to use from go.mod including full test for the functionality. This allows go projects to have a single source for go module versions. Also: * Fix lint issues in README.md. Fixes bufbuild#155

stevenh · 2023-09-07T10:44:13Z

@bufdev any feedback on this?

bufdev · 2023-09-07T14:09:36Z

Hey @stevenh - thanks for the PR, but we can't consider this at the moment. This isn't an officially supported installation path, and adding it as one isn't something we can budget for at this time unfortunately. We can revisit this in the future.

stevenh · 2023-09-07T23:10:35Z

Thanks for the feedback.

It's obviously a shame as it means people either have to not use your action or when incrementing buf versions have to change in multiple places.

In case you haven't seen it versioning tools using go.mod is one of the recommended practices details on the official go wiki in answer to How can I track tool dependencies for a module?.

bufdev · 2023-09-07T23:56:30Z

We've seen that, but we don't think it's a good practice, and isn't one we support - we've seen numerous issues with users installing tools in this manner with dependencies overlapping with their own code's dependencies, causing builds of the buf binary to use untested dependencies.

danmrichards · 2023-11-07T09:24:30Z

Hey @bufdev,

Could you elaborate on this point:

We don't recommend adding github.com/bufbuild/buf as a way to install the CLI - this installation method is not officially supported.

The installation docs do call out installation from source using go install as one of the primary install methods - https://buf.build/docs/installation

Agree with the take from @stevenh on this one. In a project where one is using the bufbuild GitHub actions alongside allowing folks to use buf locally, you can end up in a situation of maintaining the buf version in two different places. One in the GitHub action and another perhaps in a Makefile or shell script for local usage.

Another pattern we've used in the past to help with this is using GOBIN to set a local toolchain directory, avoiding the risk of overloading local versions of tools across many projects. Coupling that with being able to unify the version of buf from go.mod would make for quite a nice developer experience.

stevenh force-pushed the feat/go-version-file branch from 046bc6b to ad16c02 Compare August 15, 2023 00:57

stevenh force-pushed the feat/go-version-file branch from ad16c02 to 83406df Compare August 15, 2023 01:03

bufdev reviewed Aug 15, 2023

View reviewed changes

stevenh force-pushed the feat/go-version-file branch from 83406df to d1c8d39 Compare August 15, 2023 10:18

stevenh force-pushed the feat/go-version-file branch from d1c8d39 to 1ca48ab Compare August 15, 2023 10:18

stevenh force-pushed the feat/go-version-file branch from 1ca48ab to 8ab8b05 Compare August 15, 2023 10:20

stevenh force-pushed the feat/go-version-file branch from 8ab8b05 to 3863678 Compare August 15, 2023 10:21

feat: add go_version_file support

31d86e4

Add support for determining the version of buf to use from go.mod including full test for the functionality. This allows go projects to have a single source for go module versions. Also: * Fix lint issues in README.md. Fixes bufbuild#155

stevenh force-pushed the feat/go-version-file branch from 3863678 to 31d86e4 Compare August 15, 2023 18:41

stevenh marked this pull request as ready for review August 15, 2023 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add go_version_file support #155

feat: add go_version_file support #155

stevenh commented Aug 15, 2023 •

edited

CLAassistant commented Aug 15, 2023 •

edited

bufdev left a comment •

edited

bufdev Aug 15, 2023

stevenh Aug 15, 2023

bufdev Aug 15, 2023

stevenh Aug 15, 2023

stevenh Aug 15, 2023

bufdev Aug 15, 2023

stevenh Aug 15, 2023 •

edited

bufdev Aug 15, 2023

stevenh Aug 15, 2023

stevenh Aug 15, 2023

bufdev Aug 15, 2023

stevenh Aug 15, 2023 •

edited

bufdev Aug 15, 2023

stevenh Aug 15, 2023

bufdev Aug 15, 2023

stevenh Aug 15, 2023

stevenh commented Aug 15, 2023 •

edited

stevenh commented Sep 7, 2023

bufdev commented Sep 7, 2023

stevenh commented Sep 7, 2023

bufdev commented Sep 7, 2023

danmrichards commented Nov 7, 2023 •

edited

		@@ -1,7 +1,7 @@
		# `buf-setup-action`

		This [Action] installs the [`buf`][buf-cli] CLI in your GitHub Actions pipelines so that it can be

feat: add go_version_file support #155

Are you sure you want to change the base?

feat: add go_version_file support #155

Conversation

stevenh commented Aug 15, 2023 • edited

CLAassistant commented Aug 15, 2023 • edited

bufdev left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenh Aug 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenh Aug 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenh commented Aug 15, 2023 • edited

stevenh commented Sep 7, 2023

bufdev commented Sep 7, 2023

stevenh commented Sep 7, 2023

bufdev commented Sep 7, 2023

danmrichards commented Nov 7, 2023 • edited

stevenh commented Aug 15, 2023 •

edited

CLAassistant commented Aug 15, 2023 •

edited

bufdev left a comment •

edited

stevenh Aug 15, 2023 •

edited

stevenh Aug 15, 2023 •

edited

stevenh commented Aug 15, 2023 •

edited

danmrichards commented Nov 7, 2023 •

edited