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

[WIP] clean-up: remove cluster provisioning from e2e #3214

Closed
wants to merge 5 commits into from
Closed

[WIP] clean-up: remove cluster provisioning from e2e #3214

wants to merge 5 commits into from

Conversation

perdasilva
Copy link
Collaborator

@perdasilva perdasilva commented May 2, 2024 •
edited

Description of the change:

  • Removes cluster provisioning from the e2e test suite and moves that responsibility to the Makefile
  • Adds bingo to the project to help manage the different tools needed for the project (e.g. kind, setup_envtest, helm, etc.)

Motivation for the change:

The project is currently a bit messy:

  • e2e suite doing things it shouldn't worry about
  • Makefile with stuff many don't understand/use
  • different build tags
  • scripts that could have been a couple of lines in the Makefile

So, trying to clean it up to make it easier to maintain

Architectural changes:

Testing remarks:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Bug fixes are accompanied by regression test(s)
  • e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
  • tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
  • Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky and have an issue
  • Code is properly formatted

@perdasilva perdasilva changed the title Removes cluster provisioning from e2e and changes install namespace to operator-lifecycle-manager [wip] clean-up: remove cluster provisioning from e2e, install namespace to operator-lifecycle-manager May 2, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 2, 2024
@perdasilva perdasilva changed the title [wip] clean-up: remove cluster provisioning from e2e, install namespace to operator-lifecycle-manager [WIP] clean-up: remove cluster provisioning from e2e, install namespace to operator-lifecycle-manager May 2, 2024
@perdasilva perdasilva force-pushed the refactor/e2e branch 22 times, most recently from 64ea957 to 8fe647d Compare May 3, 2024 09:32
@perdasilva perdasilva changed the title [WIP] clean-up: remove cluster provisioning from e2e, install namespace to operator-lifecycle-manager [WIP] clean-up: remove cluster provisioning from e2e May 3, 2024
@perdasilva perdasilva changed the title [WIP] clean-up: remove cluster provisioning from e2e clean-up: remove cluster provisioning from e2e May 3, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 3, 2024
kevinrizza
kevinrizza reviewed May 3, 2024
View reviewed changes
Makefile
# Not guaranteed to have patch releases available and node image tags are full versions (i.e v1.28.0 - no v1.28, v1.29, etc.)
# The KIND_NODE_VERSION is set by getting the version of the k8s.io/client-go dependency from the go.mod
# and sets major version to "1" and the patch version to "0". For example, a client-go version of v0.28.5
# will map to a KIND_NODE_VERSION of 1.28.0
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for adding comments!

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 took a lot of this from controller-operator - so I deserve no praise here!

kevinrizza
kevinrizza reviewed May 3, 2024
View reviewed changes
Copy link
Member

@kevinrizza kevinrizza left a comment

Choose a reason for hiding this comment

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

I took a quick look and I liked what I saw. Can you update the readme in the root of the project to describe the expected developer workflow now that we're removing the deploy-local target from the makefile?

joelanford
joelanford reviewed May 3, 2024
View reviewed changes
Makefile Show resolved Hide resolved
joelanford
joelanford reviewed May 3, 2024
View reviewed changes
Makefile
@@ -53,7 +67,7 @@ unit: kubebuilder
KUBEBUILDER_ASSETS_ERR := not detected in $(KUBEBUILDER_ASSETS), to override the assets path set the KUBEBUILDER_ASSETS environment variable, for install instructions see https://pkg.go.dev/sigs.k8s.io/controller-runtime/tools/setup-envtest
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to change this to do a similar automatic setup from setup-envtest like we do in operator-controller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup yup - already have that in some commit in here somewhere. Actually, I think I might remove bingo from this PR, then add it as its own. To keep this change even smaller.

remove cluster provisioning from e2e tests
5a605f9 
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
@perdasilva perdasilva force-pushed the refactor/e2e branch 4 times, most recently from 0d5705b to 29ab405 Compare May 5, 2024 12:40
@perdasilva perdasilva changed the title clean-up: remove cluster provisioning from e2e [WIP] clean-up: remove cluster provisioning from e2e May 6, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 6, 2024
Per Goncalves da Silva added 4 commits May 6, 2024 12:00
add bingo to manage tools, e.g. kind
7ad1eb4 
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
update e2e-local target to create a kind cluster, clean, build, deplo…
5aa0eac 
…y, and test

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
fix artifact collection
f4d6aba 
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
set E2E_NODES=1
3a6508b 
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
@perdasilva
Copy link
Collaborator Author

closing in favor of #3222

@perdasilva perdasilva closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanford joelanford joelanford left review comments

@kevinrizza kevinrizza kevinrizza left review comments

@ankitathomas ankitathomas Awaiting requested review from ankitathomas

@stevekuznetsov stevekuznetsov Awaiting requested review from stevekuznetsov

Assignees
No one assigned
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@perdasilva @joelanford @kevinrizza