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

removal: helm legacy scaffold #3343

Merged
merged 1 commit into from
Jul 21, 2020
Merged

removal: helm legacy scaffold #3343

merged 1 commit into from
Jul 21, 2020

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Jul 3, 2020 •
edited
Loading

Description of the change:

  • Remove the operator-sdk new --type in favor of operator-sdk init --plugins=helm.operator-sdk.io/v1
  • Customze the flag which is binding in the new to just show that ones which are valid for Ansible
  • Remove internal/scaffold/helm
  • Update the project util to just check IsHelmOperator with the new layout
  • Removed from role_test what is done based on helm scaffolds.
  • Removed from pkg/helm/watches watch is specific to the scaffold.

What changed?

:000000 100644 00000000 400c96a2 A      changelog/fragments/cmd-legacy-helm.yaml
:000000 100644 00000000 893feb33 A      changelog/fragments/remove-legacy-helm.yaml
:100644 100644 a34ad376 68ee4d48 M      cmd/operator-sdk/add/api.go
:100644 100644 d4397c0d 96c71928 M      cmd/operator-sdk/new/cmd.go
:100644 100644 3e6b8d7d aa29cb42 M      go.mod
:100644 100644 7be4d5c2 6bc1ef95 M      internal/flags/apiflags/flags.go
:100644 100644 9b5f771b b24b9ccd M      internal/flags/apiflags/flags_test.go
:100644 000000 b9b06b0f 00000000 D      internal/scaffold/helm/api.go
:100644 000000 23b4b892 00000000 D      internal/scaffold/helm/chart.go
:100644 000000 5c00b4db 00000000 D      internal/scaffold/helm/chart_test.go
:100644 000000 a77368ae 00000000 D      internal/scaffold/helm/dockerfile.go
:100644 000000 c5c4a3e6 00000000 D      internal/scaffold/helm/init.go
:100644 000000 8c04fc5c 00000000 D      internal/scaffold/helm/operator.go
:100644 000000 b13a59b7 00000000 D      internal/scaffold/helm/role.go
:100644 000000 548dc1d5 00000000 D      internal/scaffold/helm/role_test.go
:100644 000000 c32376b2 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart-1.2.0.tgz
:100644 000000 9e5b2713 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart-1.2.3.tgz
:100644 000000 50af0317 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart/.helmignore
:100644 000000 bbe61e49 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart/Chart.yaml
:100644 000000 8fee581e 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart/templates/NOTES.txt
:100644 000000 b43406ae 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart/templates/_helpers.tpl
:100644 000000 e581bd0a 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart/templates/deployment.yaml
:100644 000000 d7d9f64e 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart/templates/ingress.yaml
:100644 000000 b8d5d516 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart/templates/service.yaml
:100644 000000 65903e18 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart/templates/serviceaccount.yaml
:100644 000000 3a7fdeb4 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart/templates/tests/test-connection.yaml
:100644 000000 5df5f37d 00000000 D      internal/scaffold/helm/testdata/testcharts/test-chart/values.yaml
:100644 100644 24203199 c6133ce0 M      internal/scaffold/role_test.go
:100644 000000 e69de29b 00000000 D      internal/scaffold/testdata/testroles/invalid_role/deploy/role.yaml
:100644 000000 40b981c6 00000000 D      internal/scaffold/testdata/testroles/valid_clusterrole/mergedRole.yaml
:100644 000000 1f877177 00000000 D      internal/scaffold/testdata/testroles/valid_role1/mergedRole.yaml
:100644 000000 705d15ae 00000000 D      internal/scaffold/testdata/testroles/valid_role2/mergedRole.yaml
:100644 000000 ebe1cb57 00000000 D      internal/scaffold/testdata/testroles/valid_role3/mergedRole.yaml
:100644 000000 c0629d8a 00000000 D      internal/scaffold/testdata/testroles/valid_role4/mergedRole.yaml
:100644 000000 a1ac9f0f 00000000 D      internal/scaffold/testdata/testroles/valid_role5/mergedRole.yaml
:100644 000000 d4809876 00000000 D      internal/scaffold/testdata/testroles/valid_role6/mergedRole.yaml
:100644 100644 d4845a40 1048d51e M      internal/util/projutil/project_util.go
:100644 100644 46c96552 6a64508e M      pkg/helm/watches/watches.go
:100644 100644 5cc4c20c cdea650c M      pkg/helm/watches/watches_test.go
:100644 100644 f96df285 6a0bc0e2 M      website/content/en/docs/cli/operator-sdk_new.md

Motivation for the change:

SDK is in a process to be integrated with KB which means that its project layouts will be aligned. More info : Integrating Kubebuilder and Operator SDK. See the PR #3421 check helm plugin scaffolding the new layout.

@camilamacedo86 camilamacedo86 added blocked language/helm Issue is related to a Helm operator project kubebuilder-integration Relates to rewriting the SDK in Kubebuilder plugin form labels Jul 3, 2020
@camilamacedo86 camilamacedo86 force-pushed the remove-new-for-helm branch 2 times, most recently from 9ea57dc to 8b62c4d Compare July 4, 2020 07:55
This was referenced Jul 10, 2020
Closed
Merged
@camilamacedo86 camilamacedo86 force-pushed the remove-new-for-helm branch 2 times, most recently from d4d2dee to 0eff21d Compare July 15, 2020 21:47
estroz
estroz suggested changes Jul 16, 2020
View reviewed changes
Copy link
Member

@estroz estroz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is a good start, but I think we should consider removing legacy commands wholesale rather than removing them piecemeal like we are with new in this PR. It creates more work.

Also there's a bunch of stuff in internal/scaffold/helm that can probably go away.

cmd/operator-sdk/main.go Outdated Show resolved Hide resolved
joelanford
joelanford requested changes Jul 16, 2020
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's remove ALL support for the legacy Helm operator scaffolding in this PR. So that would also mean removing the entire internal/scaffold/helm directory, the add api code that's specific to Helm, docs about legacy helm project, and any other references to the legacy helm project.

cmd/operator-sdk/main.go Outdated Show resolved Hide resolved
cmd/operator-sdk/new/cmd.go Outdated Show resolved Hide resolved
internal/flags/apiflags/flags.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 changed the title removal: new cmd for Helm projects WIP: removal: new cmd for Helm projects Jul 16, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2020
@camilamacedo86 camilamacedo86 changed the title WIP: removal: new cmd for Helm projects removal: helm legacy scaffold Jul 16, 2020
@openshift-ci-robot openshift-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 16, 2020
@camilamacedo86 camilamacedo86 force-pushed the remove-new-for-helm branch 3 times, most recently from 0e13277 to a5f4e0d Compare July 16, 2020 10:57
@joelanford joelanford mentioned this pull request Jul 16, 2020
Closed
92 tasks
joelanford
joelanford approved these changes Jul 17, 2020
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2020
@camilamacedo86 camilamacedo86 changed the title WIP : removal: helm legacy scaffold removal: helm legacy scaffold Jul 17, 2020
@openshift-ci-robot openshift-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 17, 2020
@camilamacedo86 camilamacedo86 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 2020
joelanford
joelanford approved these changes Jul 21, 2020
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

We should also be able to remove ./pkg/helm/watches functions Append and UpdateForResource (and their tests) since those are no longer being used in the new plugin.

Feel free to do this here or in a follow-up.

@camilamacedo86 camilamacedo86 force-pushed the remove-new-for-helm branch 2 times, most recently from 8286a3f to 86602a4 Compare July 21, 2020 08:44
estroz
estroz approved these changes Jul 21, 2020
View reviewed changes
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020
@camilamacedo86 camilamacedo86 merged commit 409f02d into operator-framework:master Jul 21, 2020
@camilamacedo86 camilamacedo86 deleted the remove-new-for-helm branch July 21, 2020 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estroz estroz estroz approved these changes

@joelanford joelanford joelanford approved these changes

@hasbro17 hasbro17 Awaiting requested review from hasbro17

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

@asmacdo asmacdo Awaiting requested review from asmacdo

@fabianvf fabianvf Awaiting requested review from fabianvf

@jmrodri jmrodri Awaiting requested review from jmrodri

Assignees

@joelanford joelanford

@estroz estroz

Labels
kubebuilder-integration Relates to rewriting the SDK in Kubebuilder plugin form language/helm Issue is related to a Helm operator project lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@camilamacedo86 @joelanford @estroz @openshift-ci-robot