API nits + refactor #704

asutula · 2020-11-11T01:17:16Z

While integrating the Powergate v1.0.0 update into Hub, I felt a few things I wanted to change here before the v1.x Powergate become widely deployed/used.

These updates mostly are related to trying to get the proto files and generated code perfect so they can become stable and we can start doing proper backward compatible proto updates moving forward (minimize breaking changes).

There are just a few small but important changes here that led to a hundred files being updated simply because of renaming of a couple things and moving the generated proto code (most file updates are generated cli docs, and every gRPC service file and client file was updated because of new import paths for generated proto code). Here is what changed:

In the gRPC API, renamed Job to StorageJob just since we're being clear about storage jobs vs retrieval jobs elsewhere in the codebase.
Moved away from "storage profile" wording in the API and client/cli to "user". I originally wanted to avoid using "user" because it implies some use case, but in reality, it is the main use case and it just makes everything easier to talk about and understand.
Updated two client methods to avoid stuttering: Client.Users.Create() and Client.Users.List() are the new names.
Adopted some improvements for proto file organization and naming. The two proto files now define the "user" API (the main api that most users use) and the "admin" api.
Moved generated proto code to the api/gen directory. This is nice because the proto types are part of the public api, and now all client and server imports come from github.com/textileio/powergate/api and its sub packages.
While adopting more proto best practices, I checked in on Buf updates and saw they now support proto code generation. This allowed me to update our Go generation to use dependencies managed by Bingo (buf, protoc-gen-go and protoc-gen-go-grpc)... no more buildtools folder and script! Much cleaner.
Using buf's code generation also made it easy to update to the new/current versions of protoc-gen-go and protoc-gen-go-grpc which I've been meaning to do for a while. Our previous code generation shell script would have needed some updating that wasn't quite clear to me, but using buf's declarative code generation configuration made this easy.

Signed-off-by: Aaron Sutula <hi@asutula.com>

asutula · 2020-11-11T16:33:47Z

.bingo/Variables.mk

+	@# Install binary/ries using Go 1.14+ build command. This is using bwplotka/bingo-controlled, separate go module with pinned dependencies.
+	@echo "(re)installing $(GOBIN)/protoc-gen-go-v1.25.0"
+	@cd .bingo && $(GO) build -modfile=protoc-gen-go.mod -o=$(GOBIN)/protoc-gen-go-v1.25.0 "google.golang.org/protobuf/cmd/protoc-gen-go"
+


protoc-gen-go and protoc-gen-go-grpc now managed by and accessed through bingo

asutula · 2020-11-11T16:37:04Z

.github/workflows/release.yml

@@ -94,7 +94,7 @@ jobs:
          replace-with: ""
      - name: Generate JS gRPC bindings
        run: |
-          ./scripts/gen-js-protos.sh ${{steps.makeversion.outputs.replaced}} . ./js-grpc
+          ./scripts/gen-js-protos.sh ${{steps.makeversion.outputs.replaced}} ./proto ./js-grpc


./proto/ is now the "root" of our proto files... ./proto/ itself is not part of the proto path, it's just the container for all protos and the actual proto paths are relative to this root.

asutula · 2020-11-11T16:39:28Z

Makefile

-protos: install-protoc clean-protos
-	PATH=$(PROTOCGENGO):$(PATH) ./scripts/protoc_gen_plugin.bash --proto_path=. --plugin_name=go --plugin_out=. --plugin_opt=plugins=grpc,paths=source_relative
+protos: $(BUF) $(PROTOC_GEN_GO) $(PROTOC_GEN_GO_GRPC) clean-protos
+	$(BUF) generate --template '{"version":"v1beta1","plugins":[{"name":"go","out":"api/gen","opt":"paths=source_relative","path":$(PROTOC_GEN_GO)},{"name":"go-grpc","out":"api/gen","opt":"paths=source_relative","path":$(PROTOC_GEN_GO_GRPC)}]}'


Using buf to generate the Go protos. buf actually supports configuring this via a config file, which is really nice, but I needed to pass the location of the buf, protoc-gen-go, and protoc-gen-go-grpc binaries managed by Bingo in dynamically, so I instead used buf's flag for passing the config in as json on the command line.

asutula · 2020-11-11T16:40:18Z

api/client/admin/admin.go

 )

 // Admin provides access to Powergate admin APIs.
 type Admin struct {
 	StorageJobs *StorageJobs
-	Profiles    *Profiles
+	Users       *Users


New "user" wording in the client. Feels much easier and makes sense.

I agree that user feels weird, but I definitely prefer it over storage profile.

asutula · 2020-11-11T16:44:11Z

api/server/admin/service.go

 	"github.com/textileio/powergate/ffs/manager"
 	"github.com/textileio/powergate/ffs/scheduler"
 	"github.com/textileio/powergate/wallet"
 )

 // Service implements the Admin API.
 type Service struct {
+	adminPb.UnimplementedAdminServiceServer


Required by the new version of gRPC to help with evolving the API moving forward. That type just provides default implementations of the service that returning unimplemented errors.

asutula · 2020-11-11T16:45:47Z

buf.yaml

-    - .
-  excludes:
-    - buildtools
+    - proto


That new root

asutula · 2020-11-11T16:46:08Z

buf.yaml

 lint:
  use:
    - DEFAULT
-  except:
-    - PACKAGE_VERSION_SUFFIX


We are doing the version suffix now

asutula · 2020-11-11T16:47:39Z

proto/powergate/admin/v1/admin.proto

@@ -1,9 +1,9 @@
 syntax = "proto3";
-package proto.admin.v1;
+package powergate.admin.v1;


Using proto as the root allows the package to be more proper like this.

jsign

Nice, I like the changes!

jsign · 2020-11-11T16:38:21Z

.bingo/Variables.mk

+	@# Install binary/ries using Go 1.14+ build command. This is using bwplotka/bingo-controlled, separate go module with pinned dependencies.
+	@echo "(re)installing $(GOBIN)/protoc-gen-go-v1.25.0"
+	@cd .bingo && $(GO) build -modfile=protoc-gen-go.mod -o=$(GOBIN)/protoc-gen-go-v1.25.0 "google.golang.org/protobuf/cmd/protoc-gen-go"
+


jsign · 2020-11-11T16:40:01Z

Makefile

-install-protoc:
-	cd buildtools && ./protocInstall.sh
-
-PROTOCGENGO=$(shell pwd)/buildtools/protoc-gen-go
-protos: install-protoc clean-protos
-	PATH=$(PROTOCGENGO):$(PATH) ./scripts/protoc_gen_plugin.bash --proto_path=. --plugin_name=go --plugin_out=. --plugin_opt=plugins=grpc,paths=source_relative
+protos: $(BUF) $(PROTOC_GEN_GO) $(PROTOC_GEN_GO_GRPC) clean-protos
+	$(BUF) generate --template '{"version":"v1beta1","plugins":[{"name":"go","out":"api/gen","opt":"paths=source_relative","path":$(PROTOC_GEN_GO)},{"name":"go-grpc","out":"api/gen","opt":"paths=source_relative","path":$(PROTOC_GEN_GO_GRPC)}]}'


Nice, the protocInstall.sh was a stretch, this looks more maintainable.

jsign · 2020-11-11T16:44:19Z

api/client/admin/admin.go

 )

 // Admin provides access to Powergate admin APIs.
 type Admin struct {
 	StorageJobs *StorageJobs
-	Profiles    *Profiles
+	Users       *Users


I agree that user feels weird, but I definitely prefer it over storage profile.

jsign · 2020-11-11T16:54:44Z

api/client/admin/storagejobs.go

@@ -3,26 +3,26 @@ package admin
 import (
 	"context"

-	proto "github.com/textileio/powergate/proto/admin/v1"
+	adminPb "github.com/textileio/powergate/api/gen/powergate/admin/v1"


Nice, the /gen subpath makes clear is gRPC releated.

asutula added 3 commits November 10, 2020 13:09

use storagejob naming, admin api names improvement

2c1c0d0

Signed-off-by: Aaron Sutula <hi@asutula.com>

few api improvements and better proto file architecture

c399023

Signed-off-by: Aaron Sutula <hi@asutula.com>

update docs

ec613ef

Signed-off-by: Aaron Sutula <hi@asutula.com>

asutula self-assigned this Nov 11, 2020

fix admin auth interceptor

8285c9e

Signed-off-by: Aaron Sutula <hi@asutula.com>

asutula changed the title ~~Last big API changes~~ API nits + refactor Nov 11, 2020

asutula marked this pull request as ready for review November 11, 2020 16:25

asutula requested a review from jsign November 11, 2020 16:26

update js proto generation

58a831d

Signed-off-by: Aaron Sutula <hi@asutula.com>

asutula commented Nov 11, 2020

View reviewed changes

buf.yaml

- .

excludes:

- buildtools

- proto

Copy link

Member Author

asutula Nov 11, 2020

Choose a reason for hiding this comment

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

That new root

asutula commented Nov 11, 2020

View reviewed changes

buf.yaml

lint:

use:

- DEFAULT

except:

- PACKAGE_VERSION_SUFFIX

Copy link

Member Author

asutula Nov 11, 2020

Choose a reason for hiding this comment

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

We are doing the version suffix now

asutula commented Nov 11, 2020

View reviewed changes

jsign approved these changes Nov 11, 2020

View reviewed changes

asutula added chore minor labels Nov 11, 2020

asutula merged commit 6fb8ee2 into master Nov 11, 2020

asutula deleted the asutula/api-nits branch November 11, 2020 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API nits + refactor #704

API nits + refactor #704

asutula commented Nov 11, 2020 •

edited

Loading

asutula Nov 11, 2020

jsign Nov 11, 2020

asutula Nov 11, 2020

asutula Nov 11, 2020

asutula Nov 11, 2020

jsign Nov 11, 2020

asutula Nov 11, 2020

asutula Nov 11, 2020

asutula Nov 11, 2020

asutula Nov 11, 2020

jsign left a comment

jsign Nov 11, 2020

jsign Nov 11, 2020

jsign Nov 11, 2020

jsign Nov 11, 2020

API nits + refactor #704

API nits + refactor #704

Conversation

asutula commented Nov 11, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsign left a 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

asutula commented Nov 11, 2020 •

edited

Loading