cmd/operator-sdk,internal: remove legacy CLI, clean up utils #3533

estroz · 2020-07-24T22:34:56Z

Description of the change:

cmd/operator-sdk: remove legacy CLI from public exposure entirely
internal: clean up utils

Motivation for the change: a lot of utils are left over from other cleanup efforts, and we don't need any legacy CLI code exposed publicly.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
Add or update relevant sections of the docs website in website/content/en/docs

joelanford

One question.

Otherwise
/lgtm

cmd/operator-sdk/main.go

estroz · 2020-07-24T23:50:49Z

Blocked by #3531

internal/plugins/helm/v1/api.go

internal/plugins/helm/v1/init.go

cmd/operator-sdk/build/cmd.go

camilamacedo86 · 2020-07-25T12:56:58Z

cmd/operator-sdk/main.go

 )

 func main() {
-	// Use the new KB CLI when running inside a Kubebuilder project with an existing PROJECT file.


Should we not still showing a msg:

if is not show the info that is required to migrate the project when has not the Project file. See: https://github.com/operator-framework/operator-sdk/pull/3539/files#diff-fc95fb53b164f1e3dc64775883195e7bR28-R35

I don't think so. SDK v1.0 has no knowledge of the legacy CLI/project layout.

It has its pros and cons, I agree. But for alpha at least I think that is valid for we do not see many new issues raised asking about the same issue.

internal/plugins/golang/v2/init.go

openshift-ci-robot · 2020-07-25T18:13:13Z

New changes are detected. LGTM label has been removed.

estroz · 2020-07-25T18:35:13Z

~~This seems to be blocked by #3433, which updates ansible-molecule tests.~~

Edit: it no longer is, since legacy build code has been left alone in this PR.

camilamacedo86

I do not think that we should remove the legacy and perform cleanups and changes as refractories in the same pr. IMO these changes should be splitted in order to allow we have visibility and revert something if required.

.

internal: clean up utils

estroz · 2020-07-27T06:34:16Z

@joelanford @camilamacedo86 removed some more legacy code but mostly kept build alone until #3433 is done, which can be removed in a follow-up.

camilamacedo86 · 2020-07-27T09:10:12Z

internal/util/projutil/project_util.go

-	return wd
-}
+// Default config file path.
+const configFile = "PROJECT"


Should be on top with the other costs.

camilamacedo86 · 2020-07-27T09:12:53Z

cmd/operator-sdk/build/cmd.go


-	if kbutil.HasProjectFile() {
+	if projutil.HasProjectFile() {


It should be removed. All projects for 1.0.0 should have HasProjectFile
The code inside of this checks is related to legacy layout.

camilamacedo86 · 2020-07-27T09:13:02Z

cmd/operator-sdk/build/cmd.go

@@ -65,7 +60,7 @@ For example:
 		"Tool to build OCI images. One of: [docker, podman, buildah]")

 	// todo: remove when the legacy layout is no longer supported
-	if !kbutil.HasProjectFile() {
+	if !projutil.HasProjectFile() {


It should be removed. All projects for 1.0.0 should have HasProjectFile
The code inside of this checks is related to legacy layout. See the too comment.

camilamacedo86

.

camilamacedo86

Just a few nits:

The build cmd still with legacy code that would be removed. See: https://github.com/operator-framework/operator-sdk/pull/3539/files#diff-438229e0c2da4b6bcda6233cd9534fe3 (note that build -go-build-args is valid just for the legacy layout and we can remove all code under //todo: remove when the legacy is no longer supported)
It is missing the fragment. E.g https://github.com/operator-framework/operator-sdk/pull/3539/files#diff-a3e7a438729f639cfbe0eb4f82c97edbR1

So, /lgtm. However, we can do it in a follow up as well.

PS.: I see more things that can get removed as well but we can do that in a follow-up. Great work btw 👍

Follow-up of #3533

openshift-ci-robot requested review from joelanford and shawn-hurley July 24, 2020 22:34

estroz force-pushed the chore/cleanup-legacy-code branch 2 times, most recently from 2045255 to 2bba2bc Compare July 24, 2020 23:20

joelanford approved these changes Jul 24, 2020

View reviewed changes

cmd/operator-sdk/main.go Outdated Show resolved Hide resolved

openshift-ci-robot assigned joelanford Jul 24, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2020

estroz changed the title ~~cmd/operator-sdk: remove legacy CLI, clean up utils~~ [WIP] cmd/operator-sdk: remove legacy CLI, clean up utils Jul 24, 2020

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 24, 2020

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2020

camilamacedo86 reviewed Jul 25, 2020

View reviewed changes

internal/plugins/helm/v1/api.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Jul 25, 2020

View reviewed changes

internal/plugins/helm/v1/init.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Jul 25, 2020

View reviewed changes

cmd/operator-sdk/build/cmd.go Show resolved Hide resolved

camilamacedo86 reviewed Jul 25, 2020

View reviewed changes

internal/plugins/golang/v2/init.go Outdated Show resolved Hide resolved

estroz force-pushed the chore/cleanup-legacy-code branch from 2bba2bc to d1ed6a9 Compare July 25, 2020 18:13

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 25, 2020

estroz changed the title ~~[WIP] cmd/operator-sdk: remove legacy CLI, clean up utils~~ cmd/operator-sdk: remove legacy CLI, clean up utils Jul 25, 2020

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 25, 2020

estroz force-pushed the chore/cleanup-legacy-code branch from d1ed6a9 to dddfab0 Compare July 25, 2020 18:15

camilamacedo86 previously requested changes Jul 25, 2020

View reviewed changes

camilamacedo86 mentioned this pull request Jul 25, 2020

removal: cleanup over legacy support #3539

Merged

estroz mentioned this pull request Jul 26, 2020

*: remove the add crd subcommand and related utilities #3547

Merged

2 tasks

estroz force-pushed the chore/cleanup-legacy-code branch 2 times, most recently from 25d2ed0 to 6a500b3 Compare July 27, 2020 05:34

cmd/operator-sdk: remove legacy CLI from public exposure entirely

36f2d1f

internal: clean up utils

estroz force-pushed the chore/cleanup-legacy-code branch from 6a500b3 to 36f2d1f Compare July 27, 2020 05:55

estroz requested review from camilamacedo86 and joelanford July 27, 2020 06:33

estroz changed the title ~~cmd/operator-sdk: remove legacy CLI, clean up utils~~ cmd/operator-sdk,internal: remove legacy CLI, clean up utils Jul 27, 2020

camilamacedo86 reviewed Jul 27, 2020

View reviewed changes

camilamacedo86 suggested changes Jul 27, 2020

View reviewed changes

camilamacedo86 approved these changes Jul 27, 2020

View reviewed changes

joelanford merged commit 62a8f29 into operator-framework:master Jul 27, 2020

camilamacedo86 added a commit that referenced this pull request Jul 30, 2020

removal: cleanup over legacy support (#3539)

6bcaff8

Follow-up of #3533

estroz deleted the chore/cleanup-legacy-code branch August 5, 2020 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/operator-sdk,internal: remove legacy CLI, clean up utils #3533

cmd/operator-sdk,internal: remove legacy CLI, clean up utils #3533

estroz commented Jul 24, 2020

joelanford left a comment

estroz commented Jul 24, 2020

camilamacedo86 Jul 25, 2020

estroz Jul 25, 2020

camilamacedo86 Jul 25, 2020 •

edited

Loading

openshift-ci-robot commented Jul 25, 2020

estroz commented Jul 25, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

estroz commented Jul 27, 2020

camilamacedo86 Jul 27, 2020

camilamacedo86 Jul 27, 2020

camilamacedo86 Jul 27, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

cmd/operator-sdk,internal: remove legacy CLI, clean up utils #3533

cmd/operator-sdk,internal: remove legacy CLI, clean up utils #3533

Conversation

estroz commented Jul 24, 2020

joelanford left a comment

Choose a reason for hiding this comment

estroz commented Jul 24, 2020

camilamacedo86 Jul 25, 2020

Choose a reason for hiding this comment

estroz Jul 25, 2020

Choose a reason for hiding this comment

camilamacedo86 Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

openshift-ci-robot commented Jul 25, 2020

estroz commented Jul 25, 2020 • edited Loading

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

estroz commented Jul 27, 2020

camilamacedo86 Jul 27, 2020

Choose a reason for hiding this comment

camilamacedo86 Jul 27, 2020

Choose a reason for hiding this comment

camilamacedo86 Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

camilamacedo86 Jul 25, 2020 •

edited

Loading

estroz commented Jul 25, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 Jul 27, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 left a comment •

edited

Loading