Refactored framework deployment utils #77373

alejandrox1 · 2019-05-02T23:49:47Z

This is the continuation of the refactoring of framework/deployment_utils.go
into framework/deployment.

Signed-off-by: Jorge Alarcon Ochoa alarcj137@gmail.com

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
This is a refactoring of framework/deployment_utils.go into framework/deployment.

Which issue(s) this PR fixes:

Part of #76206

Special notes for your reviewer:
This is a continuation of #76978

Does this PR introduce a user-facing change?:

NONE

/area e2e-test-framework
/area test
/sig testing

alejandrox1 · 2019-05-03T01:15:03Z

ptal 🙂
/assign @neolit123 @timothysc @andrewsykim

alejandrox1 · 2019-05-03T01:44:12Z

/priority backlog

neolit123

/approve
i think this is fine.

something i've noticed in the other similar PR is that we are naming the imports deployment instead of frameworkdeployment. did someone say that frameworkdeployment is the preferred pattern?

alejandrox1 · 2019-05-03T11:13:17Z

something i've noticed in the other similar PR is that we are naming the imports deployment instead of frameworkdeployment. did someone say that frameworkdeployment is the preferred pattern?

I did chose it rather arbitrarily; I wanted to avoid any name collision and make it more explicit.

Question: if we went for deploy and somewhere there was something like deployment, err := frameworkdeployment.CreateDeployment(...), and within the same scope there was another import from the deployment pkg, that would cause trouble, correct?
And if this was the case, do you think there would be a better name, something other than framework deployment that would make sense?

timothysc

change imports

test/e2e/apimachinery/aggregator.go

alejandrox1 · 2019-05-03T15:40:22Z

Changed imports from frameworkdeployment to e2edeploy
ptal, whenever possible @neolit123 @timothysc

alejandrox1 · 2019-05-03T19:01:33Z

Fixed conflicts in test/e2e/auth/audit.go from #77366 and replaced framework.Logf from test/e2e/framework/deploy with e2elog.Logf.

andrewsykim · 2019-05-03T19:09:33Z

test/e2e/framework/deployment/wrappers.go

@@ -0,0 +1,75 @@
+/*


I would prefer to collapse the files in here with util.go (I also don't love util.go as a name but that can probably be done in a follow-up PR) because aside from the fact that the methods here are just wrappers for testutils, the methods are still similar to what's in util.go. What do you think?

That makes sense. Would it be better to separate the file into public and private functions?
and change the file names to helpers.go and deployment.go?

Which methods are private?

Made the changes on the latest commit to see it better.
The couple functions in helpers.go are not used anywhere outside of framework/deployment.
logReplicaSetsOfDeployment and logPodsOfDeployment are private, while MakeDeployment is not used outside of the package.

hmm 🤔. I'd like to avoid files like helper.go that become a dumping ground for generic functions in the package. How about we put all the create/update deployment functions in fixtures.go (including MakeDeployment) and all the Wait* methods in wait.go? The log methods can go in the same file as the callers. What do you think?

I like that idea. et me give it a try

test/e2e/framework/deployment/wrappers.go

andrewsykim · 2019-05-03T20:33:18Z

test/e2e/framework/deployment/deployment.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2017 The Kubernetes Authors.
+Copyright 2019 The Kubernetes Authors.


Don't update copy right date if you're moving a file :)

Will keep in mind. thank you!

andrewsykim · 2019-05-03T20:36:47Z

test/e2e/framework/deployment/helpers.go

+
+// MakeDeployment creates a deployment definition based on the namespace. The deployment references the PVC's
+// name.  A slice of BASH commands can be supplied as args to be run by the pod
+func MakeDeployment(replicas int32, podLabels map[string]string, nodeSelector map[string]string, namespace string, pvclaims []*v1.PersistentVolumeClaim, isPrivileged bool, command string) *apps.Deployment {


makeDeployment if it's only called in this package. I think testDeployment is a better name if you're willing to rename in this PR

makes sense.

alejandrox1 · 2019-05-03T21:17:52Z

Gave it another try based on @andrewsykim's feedback

timothysc

/approve

@andrewsykim for final lgtm.

k8s-ci-robot · 2019-05-03T21:34:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alejandrox1, neolit123, timothysc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [timothysc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andrewsykim

minor comment re copyright :)

andrewsykim · 2019-05-03T21:41:57Z

test/e2e/framework/deployment/logging.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2017 The Kubernetes Authors.
+Copyright 2019 The Kubernetes Authors.


2017 - you probably made a new file here but basing it off what is seen by git

Changed it to "2017" for all files that have code straight from deployment_util.go.

2019 only if it's a new file. If you're moving a file, keep the old copy right date

Changed it to "2017" for all files that have code straight from deployment_util.go.

Thanks, general rule (that I follow at least) is new files according to git should have the current year. Any file "renamed" should keep whatever date is already there

andrewsykim · 2019-05-03T21:47:18Z

test/e2e/framework/deployment/constants.go

+	"time"
+)
+
+const (


Don't think we need a new file for holding unexported functions, what do you think about just putting these in wait.go?

Have a point there. I wanted to make it clear that the constants are used throughout the pkg but it does look good with wait.go and no "outsider" should notice it.

Yeah, I see the original intent though since it was called in both files

and fixed this

andrewsykim · 2019-05-03T21:56:50Z

test/e2e/framework/deployment/wait.go

@@ -0,0 +1,119 @@
+/*
+Copyright 2017 The Kubernetes Authors.


last nit: 2019, cause new file :)

fixed.
I think I got you mean now. It's not necessarily that the code came from a file that already existed but how Git sees it. i.e., fixtures.go is a new version of deployment_utils.go but wait.go is a new file from Git's point of view?

yes! you can use git mv to control this better if you want

This is the continuation of the refactoring of framework/deployment_utils.go into framework/deployment. Signed-off-by: Jorge Alarcon Ochoa <alarcj137@gmail.com>

andrewsykim · 2019-05-03T22:03:18Z

/lgtm

Thanks @alejandrox1!

alejandrox1 · 2019-05-03T22:19:47Z

/retest

k8s-ci-robot requested review from ixdy and janetkuo May 2, 2019 23:50

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label May 2, 2019

alejandrox1 mentioned this pull request May 2, 2019

Use Logf in test/e2e/framework/log #77359

Closed

26 tasks

alejandrox1 force-pushed the framework-deployment-refactor branch from 7dd6ccc to 383a3a3 Compare May 3, 2019 01:12

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 3, 2019

alejandrox1 changed the title ~~[WIP] Refeactored framework deployment utils~~ Refeactored framework deployment utils May 3, 2019

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 May 3, 2019

k8s-ci-robot assigned andrewsykim, neolit123 and timothysc May 3, 2019

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 3, 2019

alejandrox1 changed the title ~~Refeactored framework deployment utils~~ Refactored framework deployment utils May 3, 2019

neolit123 reviewed May 3, 2019

View reviewed changes

timothysc reviewed May 3, 2019

View reviewed changes

test/e2e/apimachinery/aggregator.go Outdated Show resolved Hide resolved

alejandrox1 force-pushed the framework-deployment-refactor branch from 383a3a3 to 8ba1a96 Compare May 3, 2019 15:28

alejandrox1 force-pushed the framework-deployment-refactor branch from 8ba1a96 to 87371fe Compare May 3, 2019 15:38

alejandrox1 force-pushed the framework-deployment-refactor branch from 87371fe to a12de05 Compare May 3, 2019 18:59

andrewsykim reviewed May 3, 2019

View reviewed changes

alejandrox1 force-pushed the framework-deployment-refactor branch from a12de05 to 01dcc22 Compare May 3, 2019 20:22

andrewsykim reviewed May 3, 2019

View reviewed changes

alejandrox1 force-pushed the framework-deployment-refactor branch from 01dcc22 to e0f0088 Compare May 3, 2019 21:14

alejandrox1 force-pushed the framework-deployment-refactor branch from e0f0088 to 234f425 Compare May 3, 2019 21:28

timothysc approved these changes May 3, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2019

andrewsykim reviewed May 3, 2019

View reviewed changes

alejandrox1 force-pushed the framework-deployment-refactor branch 3 times, most recently from dab82ec to ab36908 Compare May 3, 2019 21:54

andrewsykim reviewed May 3, 2019

View reviewed changes

Refeactored framework deployment utils

dc61906

This is the continuation of the refactoring of framework/deployment_utils.go into framework/deployment. Signed-off-by: Jorge Alarcon Ochoa <alarcj137@gmail.com>

alejandrox1 force-pushed the framework-deployment-refactor branch from ab36908 to dc61906 Compare May 3, 2019 22:01

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

k8s-ci-robot merged commit f4ce6b6 into kubernetes:master May 4, 2019

alejandrox1 deleted the framework-deployment-refactor branch May 6, 2019 15:47

+              	"time"
+              )
+              const (

Refactored framework deployment utils #77373

Refactored framework deployment utils #77373

Conversation

alejandrox1 commented May 2, 2019

alejandrox1 commented May 3, 2019

alejandrox1 commented May 3, 2019

neolit123 left a comment

Choose a reason for hiding this comment

alejandrox1 commented May 3, 2019 • edited

timothysc left a comment

Choose a reason for hiding this comment

alejandrox1 commented May 3, 2019

alejandrox1 commented May 3, 2019

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

andrewsykim May 3, 2019 • edited

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

alejandrox1 commented May 3, 2019

timothysc left a comment • edited

Choose a reason for hiding this comment

k8s-ci-robot commented May 3, 2019

andrewsykim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandrox1 May 3, 2019 • edited

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

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

andrewsykim commented May 3, 2019

alejandrox1 commented May 3, 2019

alejandrox1 commented May 3, 2019 •

edited

andrewsykim May 3, 2019 •

edited

timothysc left a comment •

edited

alejandrox1 May 3, 2019 •

edited