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

⚠️ Plugin chaining #1991

Closed
wants to merge 5 commits into from
Closed

⚠️ Plugin chaining #1991

wants to merge 5 commits into from

Conversation

Adirio
Copy link
Contributor

@Adirio Adirio commented Feb 4, 2021 •
edited
Loading

Plugin phase 1.5

Implements #1990
Closes: #2036

@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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 4, 2021
@Adirio Adirio force-pushed the plugin-phase-1.5 branch 24 times, most recently from 6ab1a65 to 1649b29 Compare February 8, 2021 07:32
@Adirio Adirio force-pushed the plugin-phase-1.5 branch 2 times, most recently from c41e7aa to d55b615 Compare February 25, 2021 07:10
@camilamacedo86 camilamacedo86 mentioned this pull request Feb 26, 2021
Closed
This was referenced Feb 26, 2021
Closed
Closed
camilamacedo86
camilamacedo86 requested changes Feb 27, 2021
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

The scenario operator-framework/operator-sdk#4581 (comment) needs also be solved before we get it merged.

@Adirio Adirio force-pushed the plugin-phase-1.5 branch 6 times, most recently from 1a7873d to 054a7b5 Compare March 1, 2021 23:24
@Adirio
Copy link
Contributor Author

Adirio commented Mar 1, 2021

After rebasing when #2037 got merged, go mod tidy is bumping the dependencies in the declarative projects when it should not. The only difference from non-declarative projects is relation to modules is the import of kubernetes-sigs/kubebuilder-declarative-pattern. Any way to prevent this in combination with go mod tidy to bump all those dependencies?

@Adirio
Copy link
Contributor Author

Adirio commented Mar 2, 2021

After rebasing when #2037 got merged, go mod tidy is bumping the dependencies in the declarative projects when it should not. The only difference from non-declarative projects is relation to modules is the import of kubernetes-sigs/kubebuilder-declarative-pattern. Any way to prevent this in combination with go mod tidy to bump all those dependencies?

I discovered the cause. go post-scaffold method was running go mod tidy and declarative post-scaffold method was running go get kubernetes-sigs/kubebuilder-declarative-pattern@.... As the go mod tidy was being run before go get ... it was importing the latest version of kubebuilder-declarative-pattern which bumped a bunch of versions. Later go get ... was defining the exact declarative pattern version but the damage was already done. Changing go get ... calls to the end of scaffold hooks instead of post-scaffold guarantees that they will all be run before go mod tidy.

@Adirio
Inject filesystem from cli instead of creating several per command
e3efd15 
Signed-off-by: Adrian Orive <[email protected]>
@Adirio
Store abstraction for persisting Config
1552d39 
- Use the cli filesystem to persist the project config
- Use PreRunE and PostRunE functions to handle configuration file loading and saving

Signed-off-by: Adrian Orive <[email protected]>
@Adirio
Implement the plugin chaining (plugin phase 1.5)
bc9576c 
Signed-off-by: Adrian Orive <[email protected]>
@Adirio
Declarative plugin
85c445e 
Signed-off-by: Adrian Orive Oneca <[email protected]>
@Adirio Adirio force-pushed the plugin-phase-1.5 branch 2 times, most recently from 8e56502 to ec0eb6f Compare March 2, 2021 23:39
@Adirio
Export and cleanup scaffolding machinery
5326b08 
- Export scaffolding machinery
- Reorganize scaffolding machinery that was in model
- Remove experimental pattern code (old plugin system)

Signed-off-by: Adrian Orive <[email protected]>
@Adirio Adirio mentioned this pull request Mar 3, 2021
Closed
@Adirio
Copy link
Contributor Author

Adirio commented Mar 3, 2021

/close

Closing this in favor of the chain of PRs #2057 - #2058 - #2059 that split this PR into several PRs to split concerns and make reviewer job easier.

@k8s-ci-robot
Copy link
Contributor

@Adirio: Closed this PR.

In response to this:

/close

Closing this in favor of the chain of PRs #2057 - #2058 - #2059 that split this PR into several PRs to split concerns and make reviewer job easier.

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/test-infra repository.

@Adirio Adirio deleted the plugin-phase-1.5 branch March 3, 2021 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 requested changes

@droot droot Awaiting requested review from droot

@mengqiy mengqiy Awaiting requested review from mengqiy

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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.

✨ Export scaffolding machinery
4 participants
@Adirio @k8s-ci-robot @christopherhein @camilamacedo86