openshift / aws-account-operator

Operator to manage pool of AWS accounts for Hive
Apache License 2.0
32 stars 75 forks source link

Make API a separate pkg #580

Closed drpaneas closed 3 years ago

drpaneas commented 3 years ago

OCM need to import the API of the operator, but it cannot do it because it has conflicts with the version of controller-runtime library we are using. As a result, this PR removes the dependency for the controller-runtime library for the API by copy-pasting the scheme go file from upstream. It also makes the API a separate sub-module in this Go project, so OCM should be able to import just that instead of the whole operator.

This PR addresses JIRA https://issues.redhat.com/browse/OSD-7287 ticket

iamkirkbater commented 3 years ago

Wow, this was really fast! Will definitely be reviewing once I get my kids to school this morning! Thanks a ton @drpaneas!!!

codecov-commenter commented 3 years ago

Codecov Report

Merging #580 (5a36c9e) into master (50eb9ff) will increase coverage by 0.14%. The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #580      +/-   ##
==========================================
+ Coverage   13.46%   13.60%   +0.14%     
==========================================
  Files          44       45       +1     
  Lines        3774     3785      +11     
==========================================
+ Hits          508      515       +7     
- Misses       3243     3247       +4     
  Partials       23       23              
Impacted Files Coverage Δ
pkg/apis/aws/v1alpha1/account_types.go 90.47% <ø> (ø)
...is/aws/v1alpha1/awsfederatedaccountaccess_types.go 100.00% <ø> (ø)
pkg/apis/aws/v1alpha1/register.go 0.00% <ø> (ø)
pkg/apis/scheme/scheme.go 63.63% <63.63%> (ø)
drpaneas commented 3 years ago

operator-sdk doesn't seem to like this:

$ operator-sdk generate crds --verbose
DEBU[0000] Debug logging is set                         
INFO[0000] Running CRD generator.                       

-: pattern ./pkg/apis/...: main module (github.com/openshift/aws-account-operator) does not contain package github.com/openshift/aws-account-operator/pkg/apis
FATA[0034] error generating CRDs from APIs in pkg/apis: error generating CRD manifests: error running CRD generator: not all generators ran successfully 

Running this via boilerplate make validate:

INFO[0000] Running CRD generator.                       
FATA[0000] error generating CRDs from APIs in pkg/apis: error generating CRD manifests: error running CRD generator: go [list -e -json -compiled=true -test=false -export=false -deps=true -find=false -tags ignore_autogenerated -- ./pkg/apis/...]: exit status 1: go: writing go.mod cache: open /go/pkg/mod/cache/download/github.com/go-bindata/go-bindata/@v/v3.1.1+incompatible.mod524366018.tmp: permission denied
go: directory pkg/apis is outside main module
go: directory pkg/apis/aws/v1alpha1 is outside main module
go: directory pkg/apis/scheme is outside main module 
make[1]: *** [op-generate] Error 1
make: *** [generate-check] Error 2

It looks like this is failing because of this:

{
    "ImportPath": "./apis/...",
    "Match": [
        "./apis/..."
    ],
    "Incomplete": true,
    "Error": {
        "ImportStack": [],
        "Pos": "",
        "Err": "pattern ./apis/...: lstat ./apis/: no such file or directory"
    }
}

This is kinda expected I guess. Same error happens for Hive, but they are not using the "operator-sdk", so they don't hit this problem.

e.g.

$ go list -e -json -compiled=true -test=false -export=false -deps=true -find=false -tags ignore_autogenerated ./apis/...
{
    "ImportPath": "./apis/...",
    "Match": [
        "./apis/..."
    ],
    "Incomplete": true,
    "Error": {
        "ImportStack": [],
        "Pos": "",
        "Err": "pattern ./apis/...: main module (github.com/openshift/hive) does not contain package github.com/openshift/hive/apis"
    }
}

they use https://book.kubebuilder.io/reference/generating-crd.html, via the controller-gen:

crd: ensure-controller-gen ensure-yq
  rm -rf ./config/crds
  (cd apis; '../$(CONTROLLER_GEN)' crd paths=./hive/v1 paths=./hiveinternal/v1alpha1 output:dir=../config/crds)
  @echo Stripping yaml breaks from CRD files
  $(foreach p,$(wildcard ./config/crds/*.yaml),$(call strip-yaml-break,$(p)))
  @echo Patching CRD files for additional static information
  $(foreach p,$(wildcard ./config/crdspatch/*.yaml),$(call patch-crd-yq,$(subst ./config/crdspatch/,./config/crds/,$(p)),$(p)))
  # Patch ClusterProvision CRD to remove the massive PodSpec def we consider an internal implementation detail:
  @echo Patching ClusterProvision CRD yaml to remove overly verbose PodSpec details:
  $(YQ) d -i config/crds/hive.openshift.io_clusterprovisions.yaml "spec.validation.openAPIV3Schema.properties.spec.properties.podSpec"
drpaneas commented 3 years ago

Going down the controller-gen route, seems to be working:

[drpaneas@pgeorgia-mac apis (changeapi ✗)]$ cd pkg/apis; controller-gen crd paths=./aws/v1alpha1 output:dir=../../deploy/crds
[drpaneas@pgeorgia-mac apis (changeapi ✗)]$ git status
On branch changeapi
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
    modified:   aws/v1alpha1/account_types.go
    modified:   aws/v1alpha1/awsfederatedaccountaccess_types.go
Untracked files:
  (use "git add <file>..." to include in what will be committed)
    ../../deploy/crds/aws.managed.openshift.io_accountclaims.yaml
    ../../deploy/crds/aws.managed.openshift.io_accountpools.yaml
    ../../deploy/crds/aws.managed.openshift.io_accounts.yaml
    ../../deploy/crds/aws.managed.openshift.io_awsfederatedaccountaccesses.yaml
    ../../deploy/crds/aws.managed.openshift.io_awsfederatedroles.yaml
no changes added to commit (use "git add" and/or "git commit -a")

If this is the preferred way of moving forward, then we need to modify:

  1. the makefile
  2. the documentation
  3. the boilerplate code
  4. the CI that triggers operator-sdk (probably doing 3 should be enough)
jharrington22 commented 3 years ago

@drpaneas this looks reasonable to me, nice work!

To fix CI add the ensure.sh call to get operator-sdk installed.

Since this make target is performing the same function as the boilerplate make target I think we're good to merge. Overwriting this is only temporary as we can update boilerplate to take these features in if all is working as expected. Slowly removing some of our dependencies on operator-sdk.

This will not effect the running on the AWS account operator, we know separating the APIs as a package already works from here hive is doing the same thing here.

iamkirkbater commented 3 years ago

/lgtm

openshift-ci[bot] commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dkeohane, drpaneas, iamkirkbater

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/aws-account-operator/blob/master/OWNERS)~~ [iamkirkbater] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment