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

[WIP] ✨: Adding ability to re-scaffold existing projects #2895

Closed
wants to merge 3 commits into from
Closed

[WIP] ✨: Adding ability to re-scaffold existing projects #2895

wants to merge 3 commits into from

Conversation

rumstead
Copy link
Contributor

Closes #2068

Relevant discussion

Description

Add tooling to help migrate existing projects to newer versions of scaffolding.

@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 Aug 25, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @rumstead. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 25, 2022
@rumstead
Copy link
Contributor Author

@camilamacedo86 / @joelanford (sorry for tagging you so much camilamcedo86 I just read the "don't tag in comments or commits"), looking for guidance here. If we want to reuse the functions that init, create, edit, etc us, we cannot put the command in the alpha package. Thoughts on how to proceed?

pkg/cli/alpha.go Outdated Show resolved Hide resolved
pkg/cli/cli.go Outdated Show resolved Hide resolved
pkg/cli/cli.go Outdated Show resolved Hide resolved
pkg/cli/scaffold.go Outdated Show resolved Hide resolved
pkg/cli/scaffold.go Outdated Show resolved Hide resolved
@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 1, 2022
pkg/cli/scaffold.go Outdated Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rumstead
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval by writing /assign @varshaprasad96 in a comment. 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. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2022
@k8s-ci-robot k8s-ci-robot removed 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 Oct 11, 2022
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 11, 2022
@rumstead
Copy link
Contributor Author

@camilamacedo86 before I go and write tests I wanted to get your take on the structure, flow, and approach.

return nil
}

func createAPI(resource resource.Resource) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of having these methods return an error from util.RunCmd I may have them return an array of strings so I can unit test easier.

log.Fatal(err)
}

projectPath := getProjectPath(cwd, projectConfig)
Copy link
Member

Choose a reason for hiding this comment

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

We need to copy the current project to an old- name dir so that the users can compare both and add the code on top of the project that was re-scaffold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would create a subdirectory with a re-scaffolded project, why would we need to touch the old project?

Copy link
Member

Choose a reason for hiding this comment

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

As a user, we create the project and add code on top.
So that to migrate the project we need to re-scaffold all and then compare the "old project" with the new scaffold so that we can re-add all code done on top again.

So, ihmo we need to do here:

a) copy the current project to another dir ( i.e. cp -R * my-operator/ old-my-operator/ )
b) then, in the current dir we will remove all files rm -rf *
c) after that we can re-scaffold all so that a user can use git OR compare my-operator/ with old-my-operator/ in order to add the code again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am suggesting we don't remove files at all. You will actually be moving the .git directory into the new folder so is git diff even possible? Taking a step back, the git diff is going to be huge and hard to review, is this a pattern that has been used before?

var args []string
args = append(args, "init")
args = append(args, generateInitArgs(store)...)
return util.RunCmd("kubebuilder init", "kubebuilder", args...)
Copy link
Member

Choose a reason for hiding this comment

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

We need to check all plugins used to initiate the project and use them if we found them tracked in the PROJECT file.
Also, if the plugin has data tracked we need to use them to pass the flags with the respective values.

See for example the case of we use the deploy image p;lugin.
You can use the samples under the test data to check it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

generateInitArgs does loop over the plugin chain. So you are saying supply the options to the plugin as well?

Copy link
Member

@camilamacedo86 camilamacedo86 Oct 22, 2022

Choose a reason for hiding this comment

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

Note that when we initiate a project we can use plugins and indeed its flags. (i.e kubebuilder init --plugins="go/v4-alpha,declarative,grafana/v1-alpha")

Also, when we run create API we can also use a plugin and pass values/flags (i.e. kubebuilder create api --group example.com --version v1alpha1 --kind Memcached --image=memcached:1.4.36-alpine --image-container-command="memcached,-m=64,-o,modern,-v" --image-container-port="11211" --run-as-user="1001" --plugins="deploy-image/v1-alpha" )

So when we re-scaffold we need to check in the PROJECT file the plugins
a) used in the layout, see:

layout:
- go.kubebuilder.io/v3
- declarative.go.kubebuilder.io/v1
- grafana.kubebuilder.io/v1-alpha

b) used to create the apis, see:

  • i.e with declarative: plugins:

    plugins:
    declarative.go.kubebuilder.io/v1:
    resources:
    - domain: testproject.org
    group: crew
    kind: Captain
    version: v1
    - domain: testproject.org
    group: crew
    kind: FirstMate
    version: v1
    - domain: testproject.org
    group: crew
    kind: Admiral
    version: v1

  • i.e. with deploy image:

    plugins:
    deploy-image.go.kubebuilder.io/v1-alpha:
    resources:
    - domain: testproject.org
    group: example.com
    kind: Memcached
    options:
    containerCommand: memcached,-m=64,-o,modern,-v
    containerPort: "11211"
    image: memcached:1.4.36-alpine
    runAsUser: "1001"
    version: v1alpha1
    - domain: testproject.org
    group: example.com
    kind: Busybox
    options:
    image: busybox:1.28
    version: v1alpha1

IMPORTANT: If we init a project with a plugin (we can check it in the layout. All plugins used in the init are under the layout) then. when we call create API or create webhook these plugins will be automatically called used. So, we just need to call in the init of all plugins under the layout and when we call create API we need to use all plugins and the values tracked under the plugins.

Copy link
Member

Choose a reason for hiding this comment

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

But we can try to make it as simple as possible. If the implementation is able to work with the deploy image, (see the sample https://github.com/kubernetes-sigs/kubebuilder/tree/master/testdata/project-v3-with-deploy-image generate ) then, it can work in the biggest part of the use cases.

@@ -29,6 +31,7 @@ const (

var alphaCommands = []*cobra.Command{
newAlphaCommand(),
alpha.NewScaffoldCommand(),
}

Copy link
Member

Choose a reason for hiding this comment

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

Could we create an e2e test to ensure this one by re-generating the scaffolds done for the project-v3-deploy-image samples under the test data for example?

@k8s-ci-robot
Copy link
Contributor

@rumstead: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubebuilder-e2e-k8s-1-19-16 dfb999c link false /test pull-kubebuilder-e2e-k8s-1-19-16
pull-kubebuilder-e2e-k8s-1-20-7 dfb999c link false /test pull-kubebuilder-e2e-k8s-1-20-7
pull-kubebuilder-e2e-k8s-1-26-0 460aeb6 link true /test pull-kubebuilder-e2e-k8s-1-26-0

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@camilamacedo86
Copy link
Member

HI @rumstead,

We created a design proposal doc to move forward with this one. See: #3244

Also, we decide to apply this one for the GS0C 2023. If accepted, a student can work on this and move forward with the work you began to try to address.

So, I hope you do not mind if we close this one. Who will be able to work on this one will still able to check this PR and see how we can use it to help out move forward. Please, feel free to reach out via slack if you need.

@Sajiyah-Salat
Copy link
Contributor

Hey @camilamacedo86 I think you should add gsoc label if this is reserved for gsoc participants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make explicit that we would like to have the migration from v3 to v4 helped by tooling
4 participants