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.

Sign up for GitHub

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

Jump to bottom

PEP 006 outputs api #2763

Merged
merged 33 commits into from
Jun 13, 2023
Merged

PEP 006 outputs api #2763

merged 33 commits into from
Jun 13, 2023

Conversation

bdegeeter
Copy link
Contributor

@bdegeeter bdegeeter commented May 16, 2023 •
edited
Loading

What does this change

This change adds a hidden argument that enables a GRPC service. This new service is the start of PEP 006 and adds a API allowing access to installation output information. This feature is only intended to be used by the operator and will require additional functional to leverage.

https://github.com/getporter/proposals/blob/main/pep/006-installation-outputs-api.md

This feature is not intended to be used directly via the porter CLI by the end user. An image is created containing a version of the porter cli with a build flag to enable the api-server command. This image is intended to be used by a future kubernetes deployment with the Porter Operator. The regular porter cli build is not built with api-server command.

Notes for the reviewer

An update to go-containerregistry required a change to existing archive integration tests. The change to this library introduces a new field to the index.json in an archive. This required the shasum for the test to be updated.

Documentation will come in as part of additional operator integration work.

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

@sgettys
Copy link
Contributor

sgettys commented May 17, 2023

We talked about putting this behind a feature flag, that's probably something we should do initially.

schristoff
schristoff reviewed May 23, 2023
View reviewed changes
cmd/porter/server.go Outdated Show resolved Hide resolved
schristoff
schristoff reviewed May 23, 2023
View reviewed changes
pkg/grpc/server.go Outdated Show resolved Hide resolved
arschles
arschles suggested changes May 24, 2023
View reviewed changes
pkg/grpc/portergrpc/context.go Show resolved Hide resolved
pkg/grpc/portergrpc/context.go Show resolved Hide resolved
pkg/grpc/portergrpc/installation.go Outdated Show resolved Hide resolved
pkg/grpc/portergrpc/installation.go Outdated Show resolved Hide resolved
pkg/grpc/portergrpc/installation.go Outdated Show resolved Hide resolved
tests/grpc/installation_test.go Outdated Show resolved Hide resolved
tests/grpc/test_utils.go Show resolved Hide resolved
tests/grpc/test_utils.go Outdated Show resolved Hide resolved
tests/grpc/test_utils.go Show resolved Hide resolved
tests/grpc/test_utils.go Show resolved Hide resolved
Brian DeGeeter and others added 15 commits June 8, 2023 15:59
@bdegeeter
parent 8ff74f4
2488fb1 
author Brian DeGeeter <b.degeeter@f5.com> 1673381173 -0800
committer Brian DeGeeter <brian@degeeter.net> 1684277785 -0700

parent 8ff74f4
author Brian DeGeeter <b.degeeter@f5.com> 1673381173 -0800
committer Brian DeGeeter <brian@degeeter.net> 1684277652 -0700

parent 8ff74f4
author Brian DeGeeter <b.degeeter@f5.com> 1673381173 -0800
committer Brian DeGeeter <brian@degeeter.net> 1684277541 -0700

chore: basic structure for grpc api

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

chore: move make targets to mage

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

wip: buf scaffolding

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: Added simple test client/server

chore: move grpc test under tests folder

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

chore: add basic grpc server test

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

chore: retire makefile

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

chore: collab on failing porter integration

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

Feat/add server to porter cmd (#2)

* wip: Saving state

* wip: Added per rpc connection

* chore: Removed debug prints

chore: Added server cli options

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: Fixed int viper conversion error

added grpc context for porter connection

feat: update proto file

chore: Generated new go grpc and moved test over to using the porter connection in the grpc context

chore: Added UT for grpc context package

Signed-off-by: Steven Gettys <s.gettys@f5.com>

fill out installation list grpc response

use PorterTest for grpc integration tests

chore: Updated grpc integration tests to inject installations into the porter test client

Signed-off-by: Steven Gettys <s.gettys@f5.com>

porter structs to grpc via json marshaling

chore: convert store install to display for grpc integ test

chore: Added UT for grpc context and installation packages

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: Consolidated duplicate test code for grpc installation service

chore: Refactored to use a single portergrpc package'

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: Renamed grpc PorterBundleServer to PorterServer

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: Checked in proto change

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: MOved around grpc package some

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: refactor grpc integ test

chore: remove porter-service cmd files

chore: refactor for integ testing

chore: add cred and param sets to installation proto

intial run outputs grpc func

refactor outputs proto to remove extra depth

chore: add test intallation opts for data driven grpc integ testing

chore: Testing out action (#3)

* chore: Testing out action

Signed-off-by: Steven Gettys <s.gettys@f5.com>

* chore: Removed the DOCKER_BUILDKIT flag

Signed-off-by: Steven Gettys <s.gettys@f5.com>

* chore: Removed docker buildkit

Signed-off-by: Steven Gettys <s.gettys@f5.com>

* chore: Added default case

Signed-off-by: Steven Gettys <s.gettys@f5.com>

---------

Signed-off-by: Steven Gettys <s.gettys@f5.com>

Bump github.com/moby/buildkit from 0.10.6 to 0.11.0 (getporter#2520)

* Bump github.com/moby/buildkit from 0.10.6 to 0.11.0

Signed-off-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Yingrong Zhao <yingrong.zhao@gmail.com>

Bump github.com/google/go-containerregistry from 0.12.1 to 0.13.0

Bumps [github.com/google/go-containerregistry](https://github.com/google/go-containerregistry) from 0.12.1 to 0.13.0.
- [Release notes](https://github.com/google/go-containerregistry/releases)
- [Changelog](https://github.com/google/go-containerregistry/blob/main/.goreleaser.yml)
- [Commits](google/go-containerregistry@v0.12.1...v0.13.0)

---
updated-dependencies:
- dependency-name: github.com/google/go-containerregistry
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Address go-restful CVE-2022-1996 (getporter#2547)

* Address go-restful CVE-2022-1996

* Upgrade go-restful to v3 which addresses CVE-2022-1996
* Upgrade containerd to v1.6.16
* Upgrade cnab-go to v1.25.0
* Upgrade docker to v23.0.0-rc.1
* Upgrade buildx to v0.10.2 which required some careful code changes to compile again

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

* Pin cnab-go to v0.25.0

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

---------

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

Bump github.com/stretchr/testify from 1.8.1 to 1.8.2

Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.8.1 to 1.8.2.
- [Release notes](https://github.com/stretchr/testify/releases)
- [Commits](stretchr/testify@v1.8.1...v1.8.2)

---
updated-dependencies:
- dependency-name: github.com/stretchr/testify
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Bump github.com/grpc-ecosystem/go-grpc-middleware from 1.3.0 to 1.4.0

Bumps [github.com/grpc-ecosystem/go-grpc-middleware](https://github.com/grpc-ecosystem/go-grpc-middleware) from 1.3.0 to 1.4.0.
- [Release notes](https://github.com/grpc-ecosystem/go-grpc-middleware/releases)
- [Commits](grpc-ecosystem/go-grpc-middleware@v1.3.0...v1.4.0)

---
updated-dependencies:
- dependency-name: github.com/grpc-ecosystem/go-grpc-middleware
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Bump github.com/spf13/afero from 1.9.4 to 1.9.5

Bumps [github.com/spf13/afero](https://github.com/spf13/afero) from 1.9.4 to 1.9.5.
- [Release notes](https://github.com/spf13/afero/releases)
- [Commits](spf13/afero@v1.9.4...v1.9.5)

---
updated-dependencies:
- dependency-name: github.com/spf13/afero
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Bump github.com/hashicorp/go-hclog from 1.4.0 to 1.5.0

Bumps [github.com/hashicorp/go-hclog](https://github.com/hashicorp/go-hclog) from 1.4.0 to 1.5.0.
- [Release notes](https://github.com/hashicorp/go-hclog/releases)
- [Commits](hashicorp/go-hclog@v1.4.0...v1.5.0)

---
updated-dependencies:
- dependency-name: github.com/hashicorp/go-hclog
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Bump github.com/google/go-containerregistry from 0.13.0 to 0.14.0

Bumps [github.com/google/go-containerregistry](https://github.com/google/go-containerregistry) from 0.13.0 to 0.14.0.
- [Release notes](https://github.com/google/go-containerregistry/releases)
- [Changelog](https://github.com/google/go-containerregistry/blob/main/.goreleaser.yml)
- [Commits](google/go-containerregistry@v0.13.0...v0.14.0)

---
updated-dependencies:
- dependency-name: github.com/google/go-containerregistry
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Bump github.com/containerd/containerd from 1.6.19 to 1.7.0

Bumps [github.com/containerd/containerd](https://github.com/containerd/containerd) from 1.6.19 to 1.7.0.
- [Release notes](https://github.com/containerd/containerd/releases)
- [Changelog](https://github.com/containerd/containerd/blob/main/RELEASES.md)
- [Commits](containerd/containerd@v1.6.19...v1.7.0)

---
updated-dependencies:
- dependency-name: github.com/containerd/containerd
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Revert "Merge pull request getporter#2653 from getporter/dependabot/go_modules/github.com/google/go-containerregistry-0.14.0"

This reverts commit 7ee355f, reversing
changes made to 09297d6.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

Bump go.mongodb.org/mongo-driver from 1.11.3 to 1.11.4

Bumps [go.mongodb.org/mongo-driver](https://github.com/mongodb/mongo-go-driver) from 1.11.3 to 1.11.4.
- [Release notes](https://github.com/mongodb/mongo-go-driver/releases)
- [Commits](mongodb/mongo-go-driver@v1.11.3...v1.11.4)

---
updated-dependencies:
- dependency-name: go.mongodb.org/mongo-driver
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

chore: basic structure for grpc api

Signed-off-by: Brian DeGeeter <brian@degeeter.net>
Signed-off-by: Steven Gettys <steven.gettys@gmail.com>

Co-authored-by: Brian DeGeeter <brian@degeeter.net>
Co-authored-by: Steven Gettys <steven.gettys@gmail.com>

chore: fix tests after rebase

Signed-off-by: Brian DeGeeter <brian@degeeter.net>

chore: tidy up go.sum

Signed-off-by: Brian DeGeeter <brian@degeeter.net>

chore: remove wip file

Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@bdegeeter
chore: update arhive test shasum to account for containerregistry man…
2c02d1f 
…ifest change (google/go-containerregistry#1551)

Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@bdegeeter
chore: address lint issues
dc1480e 
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@dependabot @bdegeeter
Bump github.com/docker/cli
9e4c1c0 
Bumps [github.com/docker/cli](https://github.com/docker/cli) from 23.0.3+incompatible to 23.0.6+incompatible.
- [Commits](docker/cli@v23.0.3...v23.0.6)

---
updated-dependencies:
- dependency-name: github.com/docker/cli
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@dependabot @bdegeeter
Bump go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp
caf1e76 
Bumps [go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp](https://github.com/open-telemetry/opentelemetry-go) from 1.14.0 to 1.15.1.
- [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases)
- [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md)
- [Commits](open-telemetry/opentelemetry-go@v1.14.0...v1.15.1)

---
updated-dependencies:
- dependency-name: go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@schristoff @bdegeeter
Update CODEOWNERS
9a451ff 
Signed-off-by: schristoff <28318173+schristoff@users.noreply.github.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@dependabot @bdegeeter
Bump github.com/docker/distribution
d1f94de 
Bumps [github.com/docker/distribution](https://github.com/docker/distribution) from 2.8.1+incompatible to 2.8.2+incompatible.
- [Release notes](https://github.com/docker/distribution/releases)
- [Commits](distribution/distribution@v2.8.1...v2.8.2)

---
updated-dependencies:
- dependency-name: github.com/docker/distribution
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@tompaana @bdegeeter
Adds custom section to output
63c65dd 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@tompaana @bdegeeter
Restores mixins in table output
8d1623a 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@tompaana @bdegeeter
Removes commented out line
8984cee 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@tompaana @bdegeeter
Minor fix
d4974ae 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@tompaana @bdegeeter
Done it the proper way this time I think
8085119 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@tompaana @bdegeeter
Modifies test data to support custom output
529fa18 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@tompaana @bdegeeter
Fixes order in expected output as it is sorted
b08a1f5 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@tompaana @bdegeeter
Updates porter explain documentation
79afd0f 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
tompaana and others added 11 commits June 8, 2023 15:59
@tompaana @bdegeeter
Adding my name to the list of contributors
109a73f 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@tompaana @bdegeeter
Fixes based in review
f1446e8 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@tompaana @bdegeeter
Grammar fix
05d34f2 
Signed-off-by: Tomi Paananen <tomi.paananen@microsoft.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@Arhell @bdegeeter
update hugo to v0.112.7
8763ddd 
Signed-off-by: Arhell <arhell333@gmail.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@dependabot @bdegeeter
Bump github.com/docker/docker
c26ca16 
Bumps [github.com/docker/docker](https://github.com/docker/docker) from 23.0.3+incompatible to 23.0.6+incompatible.
- [Release notes](https://github.com/docker/docker/releases)
- [Commits](moby/moby@v23.0.3...v23.0.6)

---
updated-dependencies:
- dependency-name: github.com/docker/docker
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@troy0820 @bdegeeter
update helm3 mixin link and changed client version in docs
9b89b32 
Signed-off-by: Troy Connor <troy0820@gmail.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@troy0820 @bdegeeter
add troy0820 to contributors
1b79fa2 
Signed-off-by: Troy Connor <troy0820@gmail.com>
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@bdegeeter
chore: basic repo structure for grpc api
311f2dd 
Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

chore: move make targets to mage

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

wip: buf scaffolding

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: Added simple test client/server

chore: move grpc test under tests folder

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

chore: add basic grpc server test

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

chore: retire makefile

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

chore: collab on failing porter integration

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>

Feat/add server to porter cmd (#2)

* wip: Saving state

* wip: Added per rpc connection

* chore: Removed debug prints

chore: Added server cli options

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: Fixed int viper conversion error

added grpc context for porter connection

feat: update proto file

chore: Generated new go grpc and moved test over to using the porter connection in the grpc context

chore: Added UT for grpc context package

Signed-off-by: Steven Gettys <s.gettys@f5.com>

fill out installation list grpc response

use PorterTest for grpc integration tests

chore: Updated grpc integration tests to inject installations into the porter test client

Signed-off-by: Steven Gettys <s.gettys@f5.com>

porter structs to grpc via json marshaling

chore: convert store install to display for grpc integ test

chore: Added UT for grpc context and installation packages

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: Consolidated duplicate test code for grpc installation service

chore: Refactored to use a single portergrpc package'

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: Renamed grpc PorterBundleServer to PorterServer

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: Checked in proto change

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: MOved around grpc package some

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: refactor grpc integ test

chore: remove porter-service cmd files

chore: refactor for integ testing

chore: add cred and param sets to installation proto

intial run outputs grpc func

refactor outputs proto to remove extra depth

chore: add test intallation opts for data driven grpc integ testing

chore: Testing out action (#3)

* chore: Testing out action

Signed-off-by: Steven Gettys <s.gettys@f5.com>

* chore: Removed the DOCKER_BUILDKIT flag

Signed-off-by: Steven Gettys <s.gettys@f5.com>

* chore: Removed docker buildkit

Signed-off-by: Steven Gettys <s.gettys@f5.com>

* chore: Added default case

Signed-off-by: Steven Gettys <s.gettys@f5.com>

---------

Signed-off-by: Steven Gettys <s.gettys@f5.com>

chore: address PR feedback

Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@bdegeeter
chore: go mod tidy
7ab9361 
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@bdegeeter
chore: fixes for integration tests
ff220e2 
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@bdegeeter
chore: fix mage imports
c5834b1 
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
bdegeeter and others added 6 commits June 8, 2023 16:00
@bdegeeter
Merge branch 'main' into pep-006-outputs-api
6b2b29c 
Signed-off-by: Brian DeGeeter <76455484+bdegeeter@users.noreply.github.com>
@bdegeeter
chore: build flags for grpc service and other PR feedback
ba15bfd 
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@bdegeeter
chore: rename grpc-server to api-server
a3d0462 
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@bdegeeter
Merge branch 'main' into pep-006-outputs-api
16277ac
@bdegeeter
chore: address lint issues
f6595ea 
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
@sgettys
docs: Added docstring to porter server struct and middleware
450a4b7 
Signed-off-by: Steven Gettys <steven.gettys@gmail.com>
schristoff
schristoff reviewed Jun 12, 2023
View reviewed changes
pkg/grpc/server.go Outdated Show resolved Hide resolved
schristoff
schristoff reviewed Jun 12, 2023
View reviewed changes
pkg/signals/shutdown.go Outdated Show resolved Hide resolved
@bdegeeter
chore: cleanup signals based on pr feedback
6d5fd72 
Signed-off-by: Brian DeGeeter <brian@degeeter.net>
schristoff
schristoff reviewed Jun 13, 2023
View reviewed changes
cmd/porter/server.go
This command starts the gRPC server for porter which is able to expose limited porter functionality via RPC.
Currently only data operations are supported, creation of resources such as installations, credential sets, or parameter sets is not supported.

A list of the supported RPCs can be found at <link?>
Copy link
Member

Choose a reason for hiding this comment

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

porter -> Porter (it is the nittiest of nits, but I only included it because I'm going to point out....) -> and <link?> ?

mage/setup/tools.go
// protoc https://github.com/protocolbuffers/protobuf/releases/download/v21.12/protoc-21.12-linux-x86_64.zip``
//TODO: add more tools
// https://github.com/bufbuild/buf/releases protoc-gen-buf-breaking, protoc-gen-buf-lint
// protoc https://github.com/protocolbuffers/protobuf/releases/download/v21.12/protoc-21.12-linux-x86_64.zip``
Copy link
Member

Choose a reason for hiding this comment

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

Is this the same TODO twice or am I not seeing the difference? If it's different then it's my b

mage/setup/tools.go
@@ -17,3 +29,67 @@ func EnsureProtobufTools() {
VersionCommand: "--version",
}))
}

// IsCommandInPath determines if a command can be called based on the current PATH.
func IsCommandInPath(cmd string) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we return one of the two here?
If we return error, then I expect for what we're doing here to be something with severity (as in, maybe we should stop if we get an error)
But if it's something we can keep running through, then just returning the bool and handling it in the caller function is probably fine

mage/setup/tools.go

func EnsureBufBuild() {
mg.Deps(EnsureProtobufTools)
if ok, _ := pkg.IsCommandAvailable("buf", "--version", "1.11.0"); ok {
Copy link
Member

Choose a reason for hiding this comment

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

Silly question - I may be misreading, feel free to correct. Are we sticking ourselves to a specific version of this? Is there anyway we could tie ourselves to 'latest' if we are, to ensure we don't have to come update this later, or is there a reason we want this specific version?

pkg/grpc/portergrpc/portergrpc.go
import (
"context"

pGRPC "get.porter.sh/porter/gen/proto/go/porterapis/porter/v1alpha1"
Copy link
Member

Choose a reason for hiding this comment

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

pGRPC -> v1alpha1 (to stay consistent with everywhere else?)

pkg/grpc/server.go

// Create a customized counter metric.
customizedCounterMetric = prometheus.NewCounterVec(prometheus.CounterOpts{
Name: "demo_server_say_hello_method_handle_count",
Copy link
Member

Choose a reason for hiding this comment

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

Should we update this name or does it not matter?

pkg/grpc/server.go
return srv, nil
}

func (s *PorterGRPCService) ListenAndServe() (*grpc.Server, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Some comments here would be majorly helpful!

tests/grpc/installation_test.go
"strconv"
"testing"

iGRPC "get.porter.sh/porter/gen/proto/go/porterapis/installation/v1alpha1"
Copy link
Member

Choose a reason for hiding this comment

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

same comment about versioning here

tests/grpc/installation_test.go
for i, gPV := range actual.GetOutputs() {
bActOut, err := pjm.Marshal(gPV)
require.NoError(t, err)
//TODO: make this not dependant on order
Copy link
Member

Choose a reason for hiding this comment

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

It looks like assert.JSONEq isn't dependent on order, is that what this TODO is referencing?

schristoff
schristoff approved these changes Jun 13, 2023
View reviewed changes
Copy link
Member

@schristoff schristoff left a comment

Choose a reason for hiding this comment

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

Leaving my notes for future selves

@schristoff schristoff merged commit 2ee4966 into getporter:main Jun 13, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgettys-alaska sgettys-alaska sgettys-alaska left review comments

@arschles arschles arschles requested changes

@schristoff schristoff schristoff approved these changes

@carolynvs carolynvs Awaiting requested review from carolynvs

@VinozzZ VinozzZ Awaiting requested review from VinozzZ

@sgettys sgettys Awaiting requested review from sgettys sgettys is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@bdegeeter @sgettys @arschles @schristoff @sgettys-alaska @tompaana @Arhell @troy0820