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

✨ Enable using setup-envtest without a separate CLI #2810

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

✨ Enable using setup-envtest without a separate CLI #2810

wants to merge 5 commits into from

Conversation

tomasaschan
Copy link
Member

@tomasaschan tomasaschan commented May 3, 2024 •
edited
Loading

This implements the proposal in #2790.

The implementation is incomplete so far; opening this mostly to show my progress and solicit feedback, as well as to avoid too many conflicts with initiatives like this one.

To do:

  • Copy existing implementation of supporting packages over into the main module
  • Implement use in the main module
  • Implement list in the main module
  • Implement cleanup in the main module
  • Implement sideload in the main module
  • Port tests for the workflows (at least the ones that make sense)
  • Clean up the setup-envtest module, removing duplicated code etc
  • Remove dependency on afero
  • Rebase onto whatever latest changes have happened in e.g. go.mod since I branched off
  • (Optional, possibly in a later PR) Re-implement the arg parsing and help parts of the main module with something a little less home-grown, probably something like https://github.com/spf13/cobra/

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 3, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tomasaschan
Once this PR has been reviewed and has the lgtm label, please assign fillzpp for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 3, 2024
@tomasaschan tomasaschan force-pushed the tomasaschan/setup-envtest-in-band branch from a12a6c3 to b3a4f85 Compare May 3, 2024 12:50
@tomasaschan
Copy link
Member Author

I will rebase this properly once #2811 merges, since I expect conflicts with that anyway.

@tomasaschan tomasaschan force-pushed the tomasaschan/setup-envtest-in-band branch from 17d784a to 3650b9b Compare June 14, 2024 08:12
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2024
@tomasaschan tomasaschan force-pushed the tomasaschan/setup-envtest-in-band branch from 3650b9b to c8aa9b3 Compare June 14, 2024 08:28
@tomasaschan
Copy link
Member Author

@sbueringer I think this is ready for an initial review now!

I still want to get rid of the dependency on afero before we merge this, so I'm keeping the PR in a draft state, but those changes should be basically independent of the major ones here. Let me know if you have any questions or would like me to somehow document the structure of the new set of packages to make reviewing easier.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2024
@tomasaschan
Copy link
Member Author

tomasaschan commented Jun 29, 2024 •
edited
Loading

I've spent some time now trying to rebuild this without a dependency on github.com/spf13/afero, and while I was initially encouraged by the existence of testing/fstest in the standard libarry, it turns out that while the afero implements a complete file system abstraction (and implementations for the two pieces we need; an OS wrapper and an in-memory one for testing), fstest only implements file actions for a read-only file system, expecting the tests to set up the appropriate input data before executing the SUT.

While it's certainly possible to build a read/write file system abstraction (on top of fstest or standalone) that supports the operations we need for setup-envtest, I feel it's probably not worth it in maintenance cost, compared to just relying on the third-party library. afero does not change rapidly and so we're unlikely to cause dependency hell issues by taking a dependency on it.

@sbueringer What do you think?

If you agree with this assessment, I can rebase again to get the latest version bumps of everything, and then this should be ready to go!

@tomasaschan tomasaschan force-pushed the tomasaschan/setup-envtest-in-band branch from c8aa9b3 to 645331e Compare June 29, 2024 17:44
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2024
tomasaschan and others added 3 commits July 29, 2024 22:21
@tomasaschan
Re-implement the setup-envtest functionality in a package
6ba031e 
There are two main points of re-implementing vs just moving the code:

1. Error handling in the old code base was based on panicking and recovering,
   where the recover basically just read out a field from the panic value and
   determined the correct exit code.

   In a package, we want more nuanced error handling, especially in order to
   allow test suites to catch the errors and surface them through their own
   reporting mechanisms.

2. There was a lot of global state in the old code base, "hidden" in the
   env.Env type that was used as a receiver for all the methods.

   This re-implementation tries to make the state more explicit, keeping only
   dependencies (like the remote client and local store) in the environment,
   while retaining the same behavior as the previous implementation.

Tests have been ported over to their respective workflow sub-packages, and
a few new ones have been added to cover cases the old test suite for one reason
or another did not. Thus, we can be fairly confident that the new implementation
does not break old functionality, even if it is a significant rewrite.
@tomasaschan
Fix bad gomega usage
69ef93e
@tomasaschan
Include go.work.sum changes
5b1508a
@tomasaschan tomasaschan force-pushed the tomasaschan/setup-envtest-in-band branch from 645331e to 5b1508a Compare July 29, 2024 20:30
@tomasaschan tomasaschan marked this pull request as ready for review July 29, 2024 20:31
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2024
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2024
@sbueringer sbueringer mentioned this pull request Aug 7, 2024
Merged
@sbueringer
Copy link
Member

@tomasaschan Just a heads up. We'll merge a PR directly after 0.19 which drops the support for GCS. Basically because we wanted to do it anyway, but also to simplify maintenance & refactorings like this PR

@tomasaschan
Copy link
Member Author

Thanks for the heads up!

Have you had time to review these changes, and in particular my last comment about keeping the afero dependency?

@sbueringer
Copy link
Member

sbueringer commented Aug 7, 2024 •
edited
Loading

Didn't get around to it yet. Just recently found time to catch up on controller-runtime in general and this is a big PR. But I'll try to look at it soon. Definitely feel free to wait with further rebases until then

@sbueringer sbueringer mentioned this pull request Sep 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FillZpp FillZpp Awaiting requested review from FillZpp

@joelanford joelanford Awaiting requested review from joelanford

@vincepri vincepri Awaiting requested review from vincepri

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomasaschan @k8s-ci-robot @sbueringer