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

Rename /internal to pkg, nested /internal/server/<group>/internal to impl #152

Merged
merged 4 commits into from
Jun 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/windows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ jobs:
- name: Build
run: |
go build -v -a -o ./bin/csi-proxy.exe ./cmd/csi-proxy
go build -v -a -o ./bin/csi-proxy-api-gen.exe ./cmd/csi-proxy-api-gen
- name: Run Windows Integration Tests
run: |
# start the CSI Proxy before running tests on windows
Expand All @@ -47,4 +48,4 @@ jobs:
uses: actions/checkout@v2
- name: Run Windows Unit Tests
run: |
go test -v -race ./internal/...
go test -v -race ./pkg/...
8 changes: 4 additions & 4 deletions cmd/csi-proxy-api-gen/generators/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const (
csiProxyAPIPath = csiProxyRootPath + "/client/api/"

// defaultServerBasePkg is the default output location for generated server files.
defaultServerBasePkg = csiProxyRootPath + "/internal/server"
defaultServerBasePkg = csiProxyRootPath + "/pkg/server"

// defaultClientBasePkg is the default output location for generated client files.
defaultClientBasePkg = csiProxyRootPath + "/client/groups"
Expand All @@ -45,7 +45,7 @@ const (
// * +csi-proxy-api-gen=groupName:<snake_case_group_name> to set the group's name
// - defaults to the package's name
// * +csi-proxy-api-gen=serverBasePkg:<pkg_path> to set the base output directory
// for generated server files - defaults to github.com/kubernetes-csi/csi-proxy/internal/server
// for generated server files - defaults to github.com/kubernetes-csi/csi-proxy/pkg/server
// * +csi-proxy-api-gen=clientBasePkg:<pkg_path> to set the base output directory
// for generated client files - defaults to github.com/kubernetes-csi/csi-proxy/client/groups
tagMarker = "+"
Expand Down Expand Up @@ -264,7 +264,7 @@ func packagesForGroup(group *groupDefinition, outputBase string) generator.Packa
},

&generator.DefaultPackage{
PackageName: "internal",
PackageName: "impl",
PackagePath: group.internalServerPkg(),
HeaderText: []byte(headerComment),

Expand Down Expand Up @@ -299,7 +299,7 @@ func packagesForGroup(group *groupDefinition, outputBase string) generator.Packa
// generate types.go if it doesn't exist and the group only has one version
if len(group.versions) == 1 && !fileExists(path.Join(outputBase, group.internalServerPkg(), "types.go")) {
pkgs = append(pkgs, &generator.DefaultPackage{
PackageName: "internal",
PackageName: "impl",
PackagePath: group.internalServerPkg(),

GeneratorList: []generator.Generator{
Expand Down
4 changes: 2 additions & 2 deletions cmd/csi-proxy-api-gen/generators/api_group_generated_gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func (g *apiGroupGeneratedGenerator) Filter(*generator.Context, *types.Type) boo
func (g *apiGroupGeneratedGenerator) Imports(*generator.Context) []string {
imports := []string{
"github.com/kubernetes-csi/csi-proxy/client/apiversion",
"srvtypes \"github.com/kubernetes-csi/csi-proxy/internal/server/types\"",
"srvtypes \"github.com/kubernetes-csi/csi-proxy/pkg/server/types\"",
g.groupDefinition.internalServerPkg(),
}

Expand All @@ -42,7 +42,7 @@ func (g *apiGroupGeneratedGenerator) Init(context *generator.Context, writer io.
snippetWriter.Do(`

// ensure the server defines all the required methods
var _ internal.ServerInterface = &Server{}
var _ impl.ServerInterface = &Server{}

func (s *Server) VersionedAPIs() []*srvtypes.VersionedAPI {
`, nil)
Expand Down
10 changes: 5 additions & 5 deletions cmd/csi-proxy-api-gen/generators/groups_and_versions.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,21 +164,21 @@ func (d *groupDefinition) serverInterfaceName() string {
}

// serverPkg returns the path of the server package, e.g.
// github.com/kubernetes-csi/csi-proxy/internal/server/<api_group_name>
// github.com/kubernetes-csi/csi-proxy/pkg/server/<api_group_name>
func (d *groupDefinition) serverPkg() string {
return fmt.Sprintf("%s/%s", d.serverBasePkg, d.name)
}

// internalServerPkg returns the path of the internal server package, e.g.
// github.com/kubernetes-csi/csi-proxy/internal/server/<api_group_name>/internal
// github.com/kubernetes-csi/csi-proxy/pkg/server/<api_group_name>/impl
func (d *groupDefinition) internalServerPkg() string {
return fmt.Sprintf("%s/%s/internal", d.serverBasePkg, d.name)
return fmt.Sprintf("%s/%s/impl", d.serverBasePkg, d.name)
}

// versionedServerPkg returns the path of the versioned server package, e.g.
// github.com/kubernetes-csi/csi-proxy/internal/server/<api_group_name>/internal/<version>
// github.com/kubernetes-csi/csi-proxy/pkg/server/<api_group_name>/impl/<version>
func (d *groupDefinition) versionedServerPkg(version string) string {
return fmt.Sprintf("%s/%s/internal/%s", d.serverBasePkg, d.name, version)
return fmt.Sprintf("%s/%s/impl/%s", d.serverBasePkg, d.name, version)
}

// versionedClientPkg returns the path of the versioned client package, e.g.
Expand Down
10 changes: 5 additions & 5 deletions cmd/csi-proxy-api-gen/generators/server_generated_gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ func (g *serverGeneratedGenerator) Init(context *generator.Context, writer io.Wr
snippetWriter.Do(`var version = apiversion.NewVersionOrPanic("$.version$")

type versionedAPI struct {
apiGroupServer internal.ServerInterface
apiGroupServer impl.ServerInterface
}

func NewVersionedServer(apiGroupServer internal.ServerInterface) internal.VersionedAPI {
func NewVersionedServer(apiGroupServer impl.ServerInterface) impl.VersionedAPI {
return &versionedAPI{
apiGroupServer: apiGroupServer,
}
Expand Down Expand Up @@ -94,8 +94,8 @@ func (g *serverGeneratedGenerator) writeWrapperFunction(callbackName string, cal
if !isVersionedVariable(param, g.version) {
continue
}
snippetWriter.Do("$.|short$ := &internal.$.|removePackage${}\n", param)
snippetWriter.Do("if err := Convert_"+g.version.Name+"_$.|removePackage$_To_internal_$.|removePackage$($.|versionedVariable$, $.|short$); err != nil {\n", param)
snippetWriter.Do("$.|short$ := &impl.$.|removePackage${}\n", param)
snippetWriter.Do("if err := Convert_"+g.version.Name+"_$.|removePackage$_To_impl_$.|removePackage$($.|versionedVariable$, $.|short$); err != nil {\n", param)
snippetWriter.Do(returnErrLine+"\n}\n", nil)
}
snippetWriter.Do("\n", nil)
Expand All @@ -119,7 +119,7 @@ func (g *serverGeneratedGenerator) writeWrapperFunction(callbackName string, cal
continue
}
snippetWriter.Do("$.|versionedVariable$ := &"+g.version.Name+".$.|removePackage${}\n", returnValue)
snippetWriter.Do("if err := Convert_internal_$.|removePackage$_To_"+g.version.Name+"_$.|removePackage$($.|short$, $.|versionedVariable$); err != nil {\n", returnValue)
snippetWriter.Do("if err := Convert_impl_$.|removePackage$_To_"+g.version.Name+"_$.|removePackage$($.|short$, $.|versionedVariable$); err != nil {\n", returnValue)
snippetWriter.Do(returnErrLine+"\n}\n", nil)
}
snippetWriter.Do("\n", nil)
Expand Down
28 changes: 14 additions & 14 deletions cmd/csi-proxy/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,20 @@ package main
import (
"flag"

diskapi "github.com/kubernetes-csi/csi-proxy/internal/os/disk"
filesystemapi "github.com/kubernetes-csi/csi-proxy/internal/os/filesystem"
iscsiapi "github.com/kubernetes-csi/csi-proxy/internal/os/iscsi"
smbapi "github.com/kubernetes-csi/csi-proxy/internal/os/smb"
sysapi "github.com/kubernetes-csi/csi-proxy/internal/os/system"
volumeapi "github.com/kubernetes-csi/csi-proxy/internal/os/volume"
"github.com/kubernetes-csi/csi-proxy/internal/server"
disksrv "github.com/kubernetes-csi/csi-proxy/internal/server/disk"
filesystemsrv "github.com/kubernetes-csi/csi-proxy/internal/server/filesystem"
iscsisrv "github.com/kubernetes-csi/csi-proxy/internal/server/iscsi"
smbsrv "github.com/kubernetes-csi/csi-proxy/internal/server/smb"
syssrv "github.com/kubernetes-csi/csi-proxy/internal/server/system"
srvtypes "github.com/kubernetes-csi/csi-proxy/internal/server/types"
volumesrv "github.com/kubernetes-csi/csi-proxy/internal/server/volume"
diskapi "github.com/kubernetes-csi/csi-proxy/pkg/os/disk"
filesystemapi "github.com/kubernetes-csi/csi-proxy/pkg/os/filesystem"
iscsiapi "github.com/kubernetes-csi/csi-proxy/pkg/os/iscsi"
smbapi "github.com/kubernetes-csi/csi-proxy/pkg/os/smb"
sysapi "github.com/kubernetes-csi/csi-proxy/pkg/os/system"
volumeapi "github.com/kubernetes-csi/csi-proxy/pkg/os/volume"
"github.com/kubernetes-csi/csi-proxy/pkg/server"
disksrv "github.com/kubernetes-csi/csi-proxy/pkg/server/disk"
filesystemsrv "github.com/kubernetes-csi/csi-proxy/pkg/server/filesystem"
iscsisrv "github.com/kubernetes-csi/csi-proxy/pkg/server/iscsi"
smbsrv "github.com/kubernetes-csi/csi-proxy/pkg/server/smb"
syssrv "github.com/kubernetes-csi/csi-proxy/pkg/server/system"
srvtypes "github.com/kubernetes-csi/csi-proxy/pkg/server/types"
volumesrv "github.com/kubernetes-csi/csi-proxy/pkg/server/volume"
"golang.org/x/sys/windows"
"golang.org/x/sys/windows/svc"
"k8s.io/klog/v2"
Expand Down
42 changes: 21 additions & 21 deletions docs/API.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ The server exposes a number of API groups, all independent of each other. Additi

APIs are defined by protobuf files; each API group should live in its own directory under `client/api/<api_group_name>` in this repo's root (e.g. `client/api/iscsi`), and then define each of its version in `client/api/<api_group_name>/<version>/api.proto` files (e.g. `client/api/iscsi/v1/api.proto`). Each `proto` file should define exactly one RPC service.

Internally, there is only one server `struct` per API group, that handles all the versions for that API group. That server is defined in this repo's `internal/server/<api_group_name>` (e.g. `internal/server/iscsi`) go package. This go package should follow the following pattern:
Internally, there is only one server `struct` per API group, that handles all the versions for that API group. That server is defined in this repo's `pkg/server/<api_group_name>` (e.g. `pkg/server/iscsi`) go package. This go package should follow the following pattern:

<a name="serverPkgTree"></a>
```
internal/server/<api_group_name>
├── internal
pkg/server/<api_group_name>
├── impl
│   └── types.go
└── server.go
```
Expand Down Expand Up @@ -59,13 +59,13 @@ message ComputeDoubleRequest{

message ComputeDoubleResponse{
int64 response = 2;

// set to true if the result overflowed
bool overflow = 3;
}
```

then `internal/server/dummy/internal/types.go` could look something like:
then `pkg/server/dummy/impl/types.go` could look something like:
```go
type ComputeDoubleRequest struct {
Input int64
Expand All @@ -76,15 +76,15 @@ type ComputeDoubleResponse struct {
Overflow bool
}
```
and then the API group's server (`internal/server/dummy/server.go`) needs to define the callbacks to handle requests for all API versions, e.g.:
and then the API group's server (`pkg/server/dummy/server.go`) needs to define the callbacks to handle requests for all API versions, e.g.:
```go
type Server struct{}

func (s *Server) ComputeDouble(ctx context.Context, request *internal.ComputeDoubleRequest, version apiversion.Version) (*internal.ComputeDoubleResponse, error) {
func (s *Server) ComputeDouble(ctx context.Context, request *impl.ComputeDoubleRequest, version apiversion.Version) (*impl.ComputeDoubleResponse, error) {
in := request.Input64
out := 2 * in
response := &internal.ComputeDoubleResponse{}

response := &impl.ComputeDoubleResponse{}

if sign(in) != sign(out) {
// overflow
Expand Down Expand Up @@ -114,14 +114,14 @@ All the boilerplate code to:
* create clients to talk to the API group and its versions
is generated automatically using [gengo](https://github.com/kubernetes/gengo).

The only caveat is that when conversions cannot be made trivially (e.g. when fields from internal and versioned `struct`s have different types), API devs need to define conversion functions. They can do that by creating an (otherwise optional) `internal/server/<api_group_name>/internal/<version>/conversion.go` file, containing functions of the form `func convert_pb_<Type>_To_internal_<Type>(in *pb.<Type>, out *internal.<Type>) error` or `func convert_internal_<Type>_To_pb_<Type>(in *internal.<Type>, out *pb.<Type>) error`; for example, in our `dummy` example above, we need to define a conversion function to account for the different fields in requests and responses from `v1alpha1` to `v1`; so `internal/server/dummy/internal/v1alpha1/conversion.go` could look like:
The only caveat is that when conversions cannot be made trivially (e.g. when fields from internal and versioned `struct`s have different types), API devs need to define conversion functions. They can do that by creating an (otherwise optional) `pkg/server/<api_group_name>/impl/<version>/conversion.go` file, containing functions of the form `func convert_pb_<Type>_To_impl_<Type>(in *pb.<Type>, out *impl.<Type>) error` or `func convert_impl_<Type>_To_pb_<Type>(in *impl.<Type>, out *pb.<Type>) error`; for example, in our `dummy` example above, we need to define a conversion function to account for the different fields in requests and responses from `v1alpha1` to `v1`; so `pkg/server/dummy/impl/v1alpha1/conversion.go` could look like:
```go
func convert_pb_ComputeDoubleRequest_To_internal_ComputeDoubleRequest(in *pb.ComputeDoubleRequest, out *internal.ComputeDoubleRequest) error {
func convert_pb_ComputeDoubleRequest_To_impl_ComputeDoubleRequest(in *pb.ComputeDoubleRequest, out *impl.ComputeDoubleRequest) error {
out.Input64 = int64(in.Input32)
return nil
}

func convert_internal_ComputeDoubleResponse_To_pb_ComputeDoubleResponse(in *internal.ComputeDoubleResponse, out *pb.ComputeDoubleResponse) error {
func convert_impl_ComputeDoubleResponse_To_pb_ComputeDoubleResponse(in *impl.ComputeDoubleResponse, out *pb.ComputeDoubleResponse) error {
i := in.Response
if i > math.MaxInt32 || i < math.MinInt32 {
return fmt.Errorf("int32 overflow for %d", i)
Expand Down Expand Up @@ -220,23 +220,23 @@ First, it looks for all API group definitions, which are either subdirectories o

Then for each API group it finds:
1. it iterates through each version subpackage, and in each looks for the `<ApiGroupName>Server` interface, and compiles the list of callbacks that the group's `Server` needs to implement as well as the list of top-level `struct`s (`*Request`s and `*Response`s)
2. it looks for an existing `internal/server/<api_group_name>/internal/types.go` file:
2. it looks for an existing `pkg/server/<api_group_name>/impl/types.go` file:
* if it exists, it checks that it contains all the expected top-level `struct`s from the previous step
* if it doesn't exist, _and_ the API group only defines one version, it auto-generates one that simply copies the protobuf `struct`s (from the previous step) - this is meant to make it easy to bootstrap a new API group
3. it generates the `internal/server/<api_group_name>/internal/types_generated.go` file, using the list of callbacks from the first step above
4. if `internal/server/<api_group_name>/server.go` doesn't exist, it generates a skeleton for it - this, too, is meant to make it easy to bootstrap new API groups
3. it generates the `pkg/server/<api_group_name>/impl/types_generated.go` file, using the list of callbacks from the first step above
4. if `pkg/server/<api_group_name>/server.go` doesn't exist, it generates a skeleton for it - this, too, is meant to make it easy to bootstrap new API groups
5. then for each version of the API:
1. it looks for an existing `internal/server/<api_group_name>/internal/<version>/conversion.go`, generates an empty one if it doesn't exist; then looks for existing conversion functions
2. it generates missing conversion functions to `internal/server/<api_group_name>/internal/<version>/conversion_generated.go`
3. it generates `internal/server/<api_group_name>/internal/<version>/server_generated.go`
6. it generates `internal/server/<api_group_name>/internal/api_group_generated.go` to list all the versioned servers it's just created
1. it looks for an existing `pkg/server/<api_group_name>/impl/<version>/conversion.go`, generates an empty one if it doesn't exist; then looks for existing conversion functions
2. it generates missing conversion functions to `pkg/server/<api_group_name>/impl/<version>/conversion_generated.go`
3. it generates `pkg/server/<api_group_name>/impl/<version>/server_generated.go`
6. it generates `pkg/server/<api_group_name>/impl/api_group_generated.go` to list all the versioned servers it's just created
7. and finally, it generates `client/groups/<api_group_name>/<version>/client_generated.go` for each version

When `csi-proxy-api-gen` has successfully run to completion, [our example API group's go package from earlier](#serverPkgTree) will look something like:
```
internal/server/<api_group_name>
pkg/server/<api_group_name>
├── api_group_generated.go
├── internal
├── impl
│   ├── types.go
│   ├── types_generated.go
│   ├── v1
Expand Down
Loading