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

feat: add support for Argo Rollouts resource #166

Merged
merged 4 commits into from
Aug 21, 2023
Merged

Conversation

dnguy078
Copy link
Collaborator

No description provided.

@github-actions github-actions bot added documentation Improvements or additions to documentation repo build labels Aug 15, 2023
@dnguy078 dnguy078 changed the title add support for rollouts feat: add support for rollouts Aug 15, 2023
@dnguy078 dnguy078 force-pushed the add-rollout-support branch from d25b0ad to fa28ec9 Compare August 15, 2023 15:24
@dnguy078 dnguy078 changed the title feat: add support for rollouts feat: add support for Argo Rollouts resource Aug 15, 2023
@coveralls
Copy link

coveralls commented Aug 15, 2023

Pull Request Test Coverage Report for Build 5904758774

  • 0 of 24 (0.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 45.784%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/builders/utils/get_selector_labels.go 0 7 0.0%
internal/builders/utils/get_pod_template_from_controller.go 0 8 0.0%
internal/builders/utils/get_rollout.go 0 9 0.0%
Totals Coverage Status
Change from base Build 5896086499: -0.5%
Covered Lines: 972
Relevant Lines: 2123

💛 - Coveralls

@dnguy078 dnguy078 force-pushed the add-rollout-support branch from e43a79f to d1c044a Compare August 15, 2023 16:51
@@ -0,0 +1,3630 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't sure how to add this CRD via kustomize. Wondering if there was a way to autogenerate this into bases folder?

Copy link
Owner

Choose a reason for hiding this comment

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

This directory is managed by the operator code and should only ever have auto-generated code. I believe you want to modify the test framework itself: https://v0-18-x.sdk.operatorframework.io/docs/golang/unit-testing/#testing-with-3rd-party-resources

Copy link
Collaborator Author

@dnguy078 dnguy078 Aug 17, 2023

Choose a reason for hiding this comment

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

I tried to write test to create a Rollout object and then create a PodAccessTemplate pointing to the Rollout and PodAccessRequest.

 failed to get API group resources: unable to retrieve the complete list of server APIs: argoproj.io/v1alpha1: the server could not find the requested resource

I read through the docs for this and tried to apply this but it seems that the cluster itself does not know what a Rollout object is when the client attempts to create it.

A workaround was to manually add the crd to the crd base path or maybe add the crd through a e2e test suite ?
https://github.com/diranged/oz/blob/main/internal/builders/podaccessbuilder/suite_test.go#L62.
I wasnt sure if theres other options ?

@diranged

@dnguy078 dnguy078 marked this pull request as ready for review August 15, 2023 17:04
@dnguy078 dnguy078 requested a review from diranged as a code owner August 15, 2023 17:04
@@ -0,0 +1,3630 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
Copy link
Owner

Choose a reason for hiding this comment

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

This directory is managed by the operator code and should only ever have auto-generated code. I believe you want to modify the test framework itself: https://v0-18-x.sdk.operatorframework.io/docs/golang/unit-testing/#testing-with-3rd-party-resources

@dnguy078 dnguy078 force-pushed the add-rollout-support branch from d1c044a to b222805 Compare August 17, 2023 04:54
@diranged
Copy link
Owner

@dnguy078 Can you add tests?

@dnguy078
Copy link
Collaborator Author

dnguy078 commented Aug 17, 2023

@dnguy078 Does https://github.com/kubernetes-sigs/kubebuilder/blob/master/docs/using_an_external_type.md help?

Not necessarily, this would require using Go's old vendoring folder and referencing the crd yaml file through there. This implies that crd.yaml is placed in a folder that has an exportable go package. Argo Rollout places their crds into a folder that has no go files https://github.com/argoproj/argo-rollouts/tree/master/manifests so we can't simply vendor this in as well.
@diranged

@dnguy078 dnguy078 force-pushed the add-rollout-support branch from 7a4a1d9 to 7e19a25 Compare August 18, 2023 05:08
@dnguy078
Copy link
Collaborator Author

dnguy078 commented Aug 18, 2023

I ended up using go mod download to be able to grab the crds from rollouts. Unable to use vendor since the crd files were place into a folder without go files.

I ended up using a mixture the approach of adding the CRD into the envtest.Environment in https://github.com/kubernetes-sigs/kubebuilder/blob/master/docs/using_an_external_type.md#prepare-for-testing. Unable to find much documentations on standards to test with third party operators beyond this. also referenced. It would be nice if operators exposed a CustomResourceDefinition struct like this.

@dnguy078 dnguy078 requested a review from diranged August 18, 2023 17:14
Copy link
Owner

@diranged diranged left a comment

Choose a reason for hiding this comment

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

one comment, then i think it looks good

var err error

// grab go mod directory with Argo rollout CRD to be installed into test environment cluster
argoCRDPath, err := extractCRDPath("github.com/argoproj/argo-rollouts", "manifests/crds")
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add this direectory to .gitignore

Copy link
Collaborator Author

@dnguy078 dnguy078 Aug 18, 2023

Choose a reason for hiding this comment

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

this directory is outside of the repo project and its installed as part of the $GOPATH/pkg via the go mod @diranged

@dnguy078 dnguy078 requested a review from diranged August 18, 2023 19:43
@dnguy078 dnguy078 merged commit 67dd4c0 into main Aug 21, 2023
@dnguy078 dnguy078 deleted the add-rollout-support branch August 21, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build documentation Improvements or additions to documentation repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants