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

Refactors the deploy directory to make config directory single source of CRD truth #1978

Merged
merged 4 commits into from
Feb 8, 2021
Merged

Refactors the deploy directory to make config directory single source of CRD truth #1978

merged 4 commits into from
Feb 8, 2021

Conversation

phantomjinx
Copy link
Contributor

@phantomjinx phantomjinx commented Feb 1, 2021 •
edited
Loading

Feature #1820: Moves the CRDs to reside in the config directory in line with operator-sdk and kustomize configurations so removing those resource from the deploy directory. With move to using operator-sdk 1.0.0+ for CRD, CSV and bundle generation, minor changes and fixes have had to be implemented to keep artifacts in line with existing requirements but also to update them where necessary.

Release Note

NONE

@astefanutti
Copy link
Member

astefanutti commented Feb 1, 2021 •
edited
Loading

Wow, that's a piece of work! I like it also brings things like automated createdAt field 😉.

It looks good overall, the main point is that it seems a different version of controller-gen is used, and produces Any types in the CRD schemas, which is not allowed in apiextensions.k8s.io/v1, and also drops some descriptions. I wonder what version of controller-gen is actually used? It seems to miss some of the Kubebuilder markers on the Flow and JSON types. If that's a newer version, it may be a matter of moving these annotations along the types hierarchy.

@astefanutti astefanutti added the area/core Core features of the integration platform label Feb 2, 2021
@phantomjinx
Copy link
Contributor Author

phantomjinx commented Feb 2, 2021 •
edited by astefanutti
Loading

It looks good overall, the main point is that it seems a different version of controller-gen is used, and produces Any types in the CRD schemas, which is not allowed in apiextensions.k8s.io/v1, and also drops some descriptions. I wonder what version of controller-gen is actually used? It seems to miss some of the Kubebuilder markers on the Flow and JSON types. If that's a newer version, it may be a matter of moving these annotations along the types hierarchy.

It was using controller-gen 0.4.0 which I had previously compiled locally. The binary uses runtime/debug/BuildInfo to determine the version number and for local compilations this is generated as 'devel'. Wrapping the module in an outer module allows the preferred version to be printed (see golang/go#29228 (comment)).

So controller-gen version fixed, version upgraded to 0.4.1 and no more surprises in the CRDs :-)

@astefanutti
Copy link
Member

Thanks @phantomjinx, that fixes it 👍🏼.

I've checked downstream, and the build calls the embed_resources.sh script directly, so it'll have to be updated to use go generate instead. The product Camel catalog generation should be alright, as the deploy directory has been kept with the community catalog.

Once CI passes, it should be good to go, and we'll update downstream build.

astefanutti
astefanutti reviewed Feb 3, 2021
View reviewed changes
script/gen_crd.sh Outdated Show resolved Hide resolved
@phantomjinx phantomjinx force-pushed the refactor-deploy-dir branch 2 times, most recently from 924e336 to 282d3d0 Compare February 3, 2021 13:40
@phantomjinx
Copy link
Contributor Author

phantomjinx commented Feb 3, 2021 •
edited by astefanutti
Loading

@astefanutti
Added an extra commit that fixes some incorrect logic, identifying Darwin and the use of sed. It looks like the wrong way around & am guessing your running mac? In which case, could you test to see if you agree?

@astefanutti
Copy link
Member

@phantomjinx thanks. Yes that inverted logic is a mistake of mine 🤦🏼! That looks much better now 👍🏼.

@phantomjinx phantomjinx force-pushed the refactor-deploy-dir branch 2 times, most recently from 2250fb3 to 84451c3 Compare February 4, 2021 18:22
@phantomjinx
Feature apache#1820: Refactor resources to make config / operator-sdk…
958d68d 
… basis for CSV

* deploy resources duplicated in config now removed

* multidir
 * Provides a vfs-gen virtual resource generator that generates from separate
   directories but doesn't reference those directories

* config/manifest/...clusterserviceversion.yaml
 * Adds in extra metadata

* deploy/olm-catalog/...
 * The creation of the CSV is retained for older OLM installations with
   bundle generation for newer ones.
 * Since operator-sdk is now used for csv generation, the metadata and
   naming has changed in minor ways:
   * camel-k-dev.package.yaml
   * renaming of deploy crd files

* pkg/base/root.go
 * Provides global var GoModDirectory that defines the location of the
   root module directory

* pkg/resources/...
 * Moves code out of deploy into resources since its independent of both
   deploy and config directories
 * Provides a ResourcesWithPrefix function for finding all resources with
   a specific prefix saving on duplicated code
 * go:generate annotation used for executing vfs-gen

* Makefile
 * go generate used for executing vfs-gen virtual resource generation
 * add_createdAt.sh adds in createdAt datetimes for CSV which is not
   generated by the operator-sdk

* script/build-olm.sh
 * Updates from operator-sdk generate command in older version to new
   command in 1.0+

* script/gen_crd.sh
 * Makes config the destination for crd generation
 * Renames the deploy/olm-catalog crd copies to match that created when
   running 	`make build-olm`

* script/gen_olm.sh
 * Migrates to new command for generating CSV using operator-sdk
 * Uses triage directory to avoid needless format changes to CRDs already
   generated by controller-gen
@phantomjinx
Fix: Makefile logic for sed, the wrong way around
3a69d17 
* On Darwin, sed requires an extra argument for the backup file whilst on
  Linux is does not.
@phantomjinx
Adds env var for running integration tests locally
a32904d 
* Specify env var KAMEL_LOCAL to install kamel from local build rather
  than fetching it from OLM (adds --olm=false to install command)
@phantomjinx
Copy link
Contributor Author

retest this please

@astefanutti astefanutti merged commit e5b5731 into apache:master Feb 8, 2021
@astefanutti
Copy link
Member

Thanks!

This was referenced Feb 11, 2021
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astefanutti astefanutti astefanutti approved these changes

Assignees
No one assigned
Labels
area/core Core features of the integration platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@phantomjinx @astefanutti