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 CI Workflow for Protocol Buffers #173

Merged
merged 8 commits into from Mar 24, 2022
Merged

Add CI Workflow for Protocol Buffers #173

merged 8 commits into from Mar 24, 2022

Conversation

bflad
Copy link
Member

@bflad bflad commented Mar 22, 2022

Closes #172

@bflad bflad requested a review from a team as a code owner March 22, 2022 14:26
Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

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

In general it looks great and a no brainer.

But I have 2 questions and a more general comment about scripts, makefiles and readmes.

tools/tools.go Show resolved Hide resolved
- 'tfprotov6/internal/tfplugin6/*'

permissions:
contents: read
Copy link
Contributor

Choose a reason for hiding this comment

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

And this doesn't apply to the filesystem where the generation below will be executed, correct?

I looked on the doc but it wasn't specified, so I assume the filesystem is always accessible for write, but then any attempt to persist the code would fail.

Also, setting only contents leaves all other permissions on the default level, right? I couldn't determine this either.

Copy link
Member Author

@bflad bflad Mar 24, 2022

Choose a reason for hiding this comment

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

contents refers to the REST API endpoints for the repository. It's not intuitive, and I'm not sure where that's documented, but its why permissions such as issues: write is necessary for modifying milestones, since those APIs are related to issues subsystem.

Assigning one of the permissions removes other permissions per this documentation:

If you specify the access for any of these scopes, all of those that are not specified are set to none.

contents: read

jobs:
protoc:
Copy link
Contributor

Choose a reason for hiding this comment

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

So, this is all good, but I think we should add a couple of things so that the knowledge isn't so "buried".

I was looking for a Makefile in the project to help doing something like make generate (to take care of the code generation). And there is no explanation about this bit of the development workflow in the README.

So I dug up the generate.sh and found:

# We do not run protoc under go:generate because we want to ensure that all
# dependencies of go:generate are "go get"-able for general dev environment
# usability. To compile all protobuf files in this repository, run
# "make protobuf" at the top-level.

So, I suspect this is a rethinking of the original approach (that I frankly don't fully understand because it wasn't explained in the generate.sh either).

Also, that bit above mentions a Makefile that doesn't seem to exist anymore.

I don't come from an "Makefile-heavy" background, but I think it would be preferable if all "developer's default actions" (not sure how to best describe it) were ready made in there. Compared to the README, that is textual description, the Makefile helps on a more "don't tell me, show me" level.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

As a matter of fact, I'd argue that the generate.sh scripts should just be replaced with Makefile commands, given that all the script does is:

  • figure out its own directory
  • call protoc

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with this change, to be honest. Will push it up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated! Please take a look and let me know what you think.

Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

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

This is excellent.

lint:
golangci-lint run ./...

# Protocol Buffers compilation is done outside of 'go generate' handling since
Copy link
Contributor

Choose a reason for hiding this comment

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

👩‍🍳 😘

Comment on lines +160 to +183
### Unit Testing

Run `go test ./...` or `make test` after any changes.

### Linting

Ensure the following tooling is installed:

- [`golangci-lint](https://golangci-lint.run/): Aggregate Go linting tool.

Run `golangci-lint run ./...` or `make lint` after any changes.

### Protocol Updates

Ensure the following tooling is installed:

- [`protoc`](https://github.com/protocolbuffers/protobuf): Protocol Buffers compiler.
- [`protoc-gen-go`](https://pkg.go.dev/google.golang.org/protobuf/cmd/protoc-gen-go): Go plugin for Protocol Buffers compiler. e.g. `go install google.golang.org/protobuf/cmd/protoc-gen-go`
- [`protoc-gen-go-grpc`](https://pkg.go.dev/google.golang.org/grpc/cmd/protoc-gen-go-grpc): Go gRPC plugin for Protocol Buffers compiler. e.g. `go install google.golang.org/grpc/cmd/protoc-gen-go-grpc`

The Protocol Buffers definitions can be found in `tfprotov5/internal/tfplugin5` and `tfprotov6/internal/tfplugin6`.

Run `make protoc` to recompile the Protocol Buffers files after any changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

👩‍🍳 😘

@bflad bflad added this to the v0.9.0 milestone Mar 24, 2022
@bflad bflad merged commit ed1368f into main Mar 24, 2022
@bflad bflad deleted the bflad-ci-protobuf branch March 24, 2022 18:23
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 CI Workflow for Protobuf Compilation
2 participants