Clean Code - Remove V1 implementation #1355

camilamacedo86 · 2020-02-05T19:07:30Z

Description

Remove the V1 code which should no longer more be used
Remove the V1 tests and testdata
Remove deprecated flags valid just for V1

Motivation

"The v1 projects are deprecated for too long and not supported since Feb 1, 2020.

Note that by removing the V1 we can:

Reduce the complexity of the code and tests

Reduce the time to check the result of tests

Remove the vendor's/deps and needs for its combability.

camilamacedo86 · 2020-02-05T20:02:22Z

/assing @droot

Adirio · 2020-02-05T21:38:44Z

I said this in several places but will say it here as its where it makes sense.

I think we should not remove v1

The code has received a huge refactor lately and v1-v2 separation is quite clear right now. Other commits such as #1348 or #1290 will help in this sense even more. I would just leave v1 as it is right now, change the deprecation warning to something like: "v1 has been completely deprecated since 1st of February, 2020, and will no longer receive any fix or update. Upgrading your projects to v2 is highly recommended. In the following link you can find a step-by-step guide on how to do it: URL". I would probably also prevent the initialization of new v1 projects.

camilamacedo86 · 2020-02-05T22:29:51Z

Hi @Adirio,

Thank you for your input. Following my comments inline.

I said this in several places but will say it here as its where it makes sense.
The code has received a huge refactor lately and v1-v2 separation is quite clear right now.

Your work was not envious. It was great and indeed allow us to continue the lifecycle doing more cleanups. And then, the work made in the V2 will still as the code that will allow us to create the V3 in the future, however, the V1 was already flagged for no longer be supported and as I said many times we should not change/update and/or care so much because we knew that at some point it would be removed.

Why should we keep a code, tests, mocks, scripts, vendor and etc if they should no longer be used/updated/changed?

Note that by removing the V1 we can:

Reduce the complexity of the code and tests
Reduce the time to check the result of tests
Remove the vendor's/deps and needs for its combability.

So, IMO by removing all unnecessary V1 implementation it can simplify a lot. However, in POV we can keep the updated command to allow users are able to upgrade their projects to V2

@droot @mengqiy @DirectXMan12 WDYT?
PS.: PR is in WIP yet. ( I will probably wait for the plugin pr be merged o finish and to ask review on it)

Adirio · 2020-02-06T09:59:41Z

I said this in several places but will say it here as its where it makes sense.
The code has received a huge refactor lately and v1-v2 separation is quite clear right now.

Your work was not envious. It was great and indeed allow us to continue the lifecycle doing more cleanups. And then, the work made in the V2 will still as the code that will allow us to create the V3 in the future, however, the V1 was already flagged for no longer be supported and as I said many times we should not change/update and/or care so much because we knew that at some point it would be removed.

I'm not worried about my code dissapearing. 😄 The point I was trying to make is that the situation when it was decided that v1 should be deprecated (3 months ago) and now is quite different, so despite being deprecated we may not need to remove the code completely.

Why should we keep a code, tests, mocks, scripts, vendor and etc if they should no longer be used/updated/changed?

Note that by removing the V1 we can:

Reduce the complexity of the code and tests

Reduce the time to check the result of tests

Remove the vendor's/deps and needs for its combability.

I do agree that we should remove the v1 tests from golden_test.sh, test.sh, and test_e2e.sh and thus the testdata/gopath dir. But I think that that is enough. I would keep the code for v1. Right now I would not say that v1 makes the code more complex outside of the mentioned tests.

Adirio

There are 22 files deleted from testdata/package-v2* and both go.mod files which have three less lines. I think these removals are due to the make all test removal I mention in one of my comments. Please readd that line and run generate_golden.sh again to see if they get removed in that case.

.travis.yml

CONTRIBUTING.md

cmd/init.go

cmd/internal/config.go

cmd/main.go

cmd/webhook.go

generate_golden.sh

test.sh

Adirio

Forgot about this two lines

test.sh

Adirio

You are using this PR (removing v1) to introduce another completly different change (removing force flag and enforcing the resource to be scaffolded if it doesn't exist. This needs to be done in another PR not here.

Aside from that, a few NITs.

cmd/internal/config.go

cmd/main.go

generate_golden.sh

k8s-ci-robot · 2020-02-15T14:09:07Z

@camilamacedo86: GitHub didn't allow me to assign the following users: diroot.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @DirectXMan12
/assign @diroot
/assign @mengqiy

WDYT?

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.

internal/config/config.go

pkg/scaffold/api.go

pkg/scaffold/init.go

estroz · 2020-04-29T15:43:59Z

pkg/scaffold/update.go

@@ -20,7 +20,7 @@ import (
 	"sigs.k8s.io/kubebuilder/pkg/model"


I think we can fully remove the update command right @mengqiy?

Or at least remove it's logic and check updateCmd.HasSubCommands() before adding it, like kubebuilder does with alpha.

It seems this file is no longer needed.

I agree but IHMO we should remove the update command in a follow-up PR.
Then, if we need for any reason to do a revert will be easier.
The scope here is NOT to remove commands or deprecated args. The goal here is just clean up and remove the v1 code source.

test/e2e/v2/e2e_suite.go

test/e2e/v3/e2e_suite.go

estroz · 2020-04-29T15:47:16Z

pkg/scaffold/internal/templates/gopkg.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.


This file should be removed.

It is used in the update.

See #1355 (comment).

I agree that we could/should remove. After thinking about and checking it I just think that it should be better done in a follow-up PR. note that we need to check/change the docs and all places that are speaking about it. So, shows out of the scope. WDYT? Could we agree in to do it a follow-up PR?

mengqiy · 2020-04-30T02:02:17Z

test/e2e/v3/e2e_suite.go

@@ -67,7 +67,7 @@ var _ = Describe("kubebuilder", func() {
 			err := kbc.Init(
 				"--project-version", "3-alpha",
 				"--domain", kbc.Domain,
-				"--dep=false")
+				"--fetch-deps=false")


I may miss something, but why do we switch to use the new flag? What's the motiviation?

In some point, we received a suggestion to remove the deprecated flags. However, I revert this suggestion. It is not part of the scope of this PR. Here we are only removing the V1 code. So, good catcher. It is using dep again.

Is —dep not deprecated? It says so in code comments. Was this depreciation unrelated to the v2 change?

it is deprecated but still valid for v2.
We can remove the flag for V3 since is deprecated and I think we should do that.
However, it shows not part of the scope of this one and then, I think that would be better we have a specific or for that.

pkg/cli/cli.go

estroz · 2020-05-01T16:26:40Z

test/e2e/v2/e2e_suite.go

@@ -65,7 +65,6 @@ var _ = Describe("kubebuilder", func() {
 			var controllerPodName string
 			By("init v2 project")
 			err := kbc.Init(
-				"--project-version", "2",


This is still necessary now that the default project version is 3-alpha.

estroz

Two nits then lgtm

estroz · 2020-05-01T16:28:19Z

pkg/model/config/project_type.go

@@ -0,0 +1,12 @@
+package config


Should be removed.

estroz

/lgtm

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

k8s-ci-robot requested review from droot and mengqiy February 5, 2020 19:07

camilamacedo86 force-pushed the removeV1code branch 3 times, most recently from 6fafcb9 to b3c3950 Compare February 5, 2020 20:00

camilamacedo86 mentioned this pull request Feb 5, 2020

Named arguments for path formatting #1252

Merged

Adirio suggested changes Feb 6, 2020

View reviewed changes

test.sh Outdated Show resolved Hide resolved

test.sh Outdated Show resolved Hide resolved

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

camilamacedo86 force-pushed the removeV1code branch from b3c3950 to 47009c5 Compare February 15, 2020 12:27

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2020

camilamacedo86 force-pushed the removeV1code branch from 47009c5 to 0eb3eb8 Compare February 15, 2020 13:05

Adirio suggested changes Feb 15, 2020

View reviewed changes

cmd/internal/config.go Outdated Show resolved Hide resolved

cmd/main.go Outdated Show resolved Hide resolved

cmd/main.go Outdated Show resolved Hide resolved

generate_golden.sh Outdated Show resolved Hide resolved

camilamacedo86 force-pushed the removeV1code branch 2 times, most recently from cba7f97 to 17578da Compare February 15, 2020 14:07

camilamacedo86 changed the title ~~WIP - Clean Code - Remove V1~~ Clean Code - Remove V1 testdata and code impl Feb 15, 2020

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 Feb 15, 2020

This comment has been minimized.

Sign in to view

k8s-ci-robot assigned DirectXMan12 and mengqiy Feb 15, 2020

camilamacedo86 force-pushed the removeV1code branch from 17578da to f6ecbe2 Compare February 15, 2020 14:11

camilamacedo86 changed the title ~~Clean Code - Remove V1 testdata and code impl~~ Clean Code - Remove V1 implementation Feb 15, 2020

camilamacedo86 force-pushed the removeV1code branch 4 times, most recently from 8a7fb2e to f2fe2b1 Compare April 29, 2020 10:22

estroz suggested changes Apr 29, 2020

View reviewed changes

camilamacedo86 force-pushed the removeV1code branch 2 times, most recently from 6e8987d to 0115566 Compare April 29, 2020 23:23

camilamacedo86 requested a review from estroz April 29, 2020 23:24

camilamacedo86 force-pushed the removeV1code branch 2 times, most recently from d17db6b to 51b539c Compare April 30, 2020 01:11

mengqiy reviewed Apr 30, 2020

View reviewed changes

camilamacedo86 force-pushed the removeV1code branch 2 times, most recently from f5855de to 76c13b3 Compare April 30, 2020 09:07

camilamacedo86 requested a review from mengqiy April 30, 2020 09:38

estroz reviewed Apr 30, 2020

View reviewed changes

pkg/cli/cli.go Outdated Show resolved Hide resolved

camilamacedo86 force-pushed the removeV1code branch from 76c13b3 to bcf261b Compare May 1, 2020 02:26

camilamacedo86 requested a review from estroz May 1, 2020 02:27

camilamacedo86 mentioned this pull request May 1, 2020

upgrade controller-runtime from v0.5.0 to v0.6.0 and k8s api from v0.17.2 to v0.18.2 #1483

Closed

camilamacedo86 force-pushed the removeV1code branch from bcf261b to f276961 Compare May 1, 2020 11:24

estroz reviewed May 1, 2020

View reviewed changes

estroz suggested changes May 1, 2020

View reviewed changes

estroz reviewed May 1, 2020

View reviewed changes

pkg/model/config/project_type.go Outdated

@@ -0,0 +1,12 @@

package config

Copy link

Contributor

estroz May 1, 2020

Choose a reason for hiding this comment

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

Should be removed.

camilamacedo86 force-pushed the removeV1code branch from f276961 to 08861e7 Compare May 1, 2020 17:02

clean v1: remove the v1 code which is no longer supported

7bdcf98

camilamacedo86 force-pushed the removeV1code branch from 08861e7 to 7bdcf98 Compare May 1, 2020 17:06

estroz approved these changes May 1, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2020

k8s-ci-robot merged commit 1bc124a into kubernetes-sigs:master May 1, 2020

camilamacedo86 deleted the removeV1code branch May 1, 2020 17:34

camilamacedo86 mentioned this pull request Feb 18, 2021

New Release - proposing 3.0.0 stable #2029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean Code - Remove V1 implementation #1355

Clean Code - Remove V1 implementation #1355

camilamacedo86 commented Feb 5, 2020 •

edited

Loading

camilamacedo86 commented Feb 5, 2020

Adirio commented Feb 5, 2020 •

edited

Loading

camilamacedo86 commented Feb 5, 2020 •

edited

Loading

Adirio commented Feb 6, 2020

Adirio left a comment

Adirio left a comment

Adirio left a comment

This comment has been minimized.

k8s-ci-robot commented Feb 15, 2020

estroz Apr 29, 2020

estroz Apr 29, 2020

mengqiy Apr 30, 2020

camilamacedo86 Apr 30, 2020

estroz Apr 29, 2020

camilamacedo86 Apr 29, 2020

estroz Apr 29, 2020

camilamacedo86 Apr 29, 2020 •

edited

Loading

mengqiy Apr 30, 2020

camilamacedo86 Apr 30, 2020 •

edited

Loading

estroz Apr 30, 2020

camilamacedo86 Apr 30, 2020

estroz May 1, 2020

estroz left a comment

estroz May 1, 2020

estroz left a comment

		@@ -20,7 +20,7 @@ import (
		"sigs.k8s.io/kubebuilder/pkg/model"

		@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
		limitations under the License.

Clean Code - Remove V1 implementation #1355

Clean Code - Remove V1 implementation #1355

Conversation

camilamacedo86 commented Feb 5, 2020 • edited Loading

camilamacedo86 commented Feb 5, 2020

Adirio commented Feb 5, 2020 • edited Loading

I think we should not remove v1

camilamacedo86 commented Feb 5, 2020 • edited Loading

Adirio commented Feb 6, 2020

Adirio left a comment

Choose a reason for hiding this comment

Adirio left a comment

Choose a reason for hiding this comment

Adirio left a comment

Choose a reason for hiding this comment

This comment has been minimized.

k8s-ci-robot commented Feb 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Feb 5, 2020 •

edited

Loading

Adirio commented Feb 5, 2020 •

edited

Loading

camilamacedo86 commented Feb 5, 2020 •

edited

Loading

camilamacedo86 Apr 29, 2020 •

edited

Loading

camilamacedo86 Apr 30, 2020 •

edited

Loading