Skip to content

Refactor e2e framework to remove globals #2037

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
Merged
merged 3 commits into from
Nov 5, 2019
Merged

Refactor e2e framework to remove globals #2037

merged 3 commits into from
Nov 5, 2019

Conversation

joelanford
Copy link
Member

Description of the change:
This PR refactors the e2e framework to remove unnecessary global variables.

Motivation for the change:
Global Variables Are Bad

@joelanford joelanford added test-framework kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Oct 10, 2019
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 10, 2019
@camilamacedo86
Copy link
Contributor

/test e2e-aws-go
/test e2e-aws-subcommand
/test e2e-aws-ansible

@camilamacedo86
Copy link
Contributor

/test e2e-aws-go
/test e2e-aws-subcommand

@joelanford
Copy link
Member Author

@camilamacedo86 Looks like a legitimate CI failure, so I think I need to make more changes to get this to pass.

@camilamacedo86
Copy link
Contributor

HI @joelanford,

@camilamacedo86 Looks like a legitimate CI failure, so I think I need to make more changes to get this to pass.

Unfortunately, yes. I checked the logs now. Since it usually stucks I try 2 times see if pass if not then I check the logs.

camilamacedo86
camilamacedo86 reviewed Oct 21, 2019
View reviewed changes
pkg/test/resource_creator.go
@@ -70,26 +66,26 @@ func (ctx *TestCtx) createFromYAML(yamlFile []byte, skipIfExists bool, cleanupOp
return fmt.Errorf("failed to unmarshal object spec: (%v)", err)
}
obj.SetNamespace(namespace)
err = Global.Client.Create(goctx.TODO(), obj, cleanupOptions)
err = ctx.client.Create(goctx.TODO(), obj, cleanupOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

The code shows great and is passing in the test which should enough to ensure it.
Just a nit, it shows be a break changing. Is not it missing the changelog? Also, should not it goes to a migrate doc? Could we work with an unreleased section as well? WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is a breaking change. Did I break something without realizing it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to change the doc since it says for the users use the global one and will it framework.Global will still be working? If not, then I think is a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. framework.Global is still there and configured the same way. I think there is no change to any public API or functionality at all. Just an internal refactoring to get rid of the unexported global variables.

camilamacedo86
camilamacedo86 approved these changes Oct 28, 2019
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 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.

Since it just affects the internal tests then the CI should be enough to ensure the qa.
It is just removing the global vars and adding as an attributes of the struct.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2019
jmrodri
jmrodri approved these changes Nov 5, 2019
View reviewed changes
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

internal/util/projutil/exec.go
@@ -29,7 +29,7 @@ func ExecCmd(cmd *exec.Cmd) error {
cmd.Stderr = os.Stderr
log.Debugf("Running %#v", cmd.Args)
if err := cmd.Run(); err != nil {
return fmt.Errorf("failed to exec %#v: %v", cmd.Args, err)
return fmt.Errorf("failed to exec %#v: %w", cmd.Args, err)
Copy link
Member

Choose a reason for hiding this comment

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

👍

@jmrodri jmrodri merged commit e03cde6 into operator-framework:master Nov 5, 2019
nikhil-thomas added a commit to nikhil-thomas/operator-sdk that referenced this pull request Nov 20, 2019
@nikhil-thomas
Add --watch-namespace to operator-sdk test local sub-command
bdef78a 
This patch resolves mergeconflicts in cherry-pick ea8caeb
and recent refactoring in operator-sdk/pkg/test
operator-framework#2037

Add --watch-namespace flag to `operator-sdk test local <test-dir> --up-local` command

`--deploy-namespace` flag specifies namespace where namespaced resources are deployed for testing

`--watch-namespace` flag specifies the namespace which operator watches during testing. That is the value
of `WATCH_NAMESPACE` envvar

**the behavior of `--watch-namespace` flags is as follows**

when not set
    `WATCH_NAMESPACE` is set to deploy namespace.
    which means when `--watch-namespace` is not set, watch-namespace == deploy-namespace
    operator watched deploy-namespace during testing

`--watch-namespace=foo`
    `WATCH_NAMESPACES=foo` operator watches namespace `foo` during testing

`--watch-namespace=""`
    `WATCH_NAMESPACES=""` operator watches all namespaces during testing

Update tests

Signed-off-by: Nikhil Thomas <[email protected]>
(cherry picked from commit ea8caeb)
Signed-off-by: Nikhil Thomas <[email protected]>
nikhil-thomas added a commit to nikhil-thomas/operator-sdk that referenced this pull request Nov 20, 2019
@nikhil-thomas
Add --watch-namespace to operator-sdk test local sub-command
6cad098 
This patch resolves mergeconflicts in cherry-pick ea8caeb
and recent refactoring in operator-sdk/pkg/test
operator-framework#2037

Add --watch-namespace flag to `operator-sdk test local <test-dir> --up-local` command

`--deploy-namespace` flag specifies namespace where namespaced resources are deployed for testing

`--watch-namespace` flag specifies the namespace which operator watches during testing. That is the value
of `WATCH_NAMESPACE` envvar

**the behavior of `--watch-namespace` flags is as follows**

when not set
    `WATCH_NAMESPACE` is set to deploy namespace.
    which means when `--watch-namespace` is not set, watch-namespace == deploy-namespace
    operator watched deploy-namespace during testing

`--watch-namespace=foo`
    `WATCH_NAMESPACES=foo` operator watches namespace `foo` during testing

`--watch-namespace=""`
    `WATCH_NAMESPACES=""` operator watches all namespaces during testing

Update tests

Signed-off-by: Nikhil Thomas <[email protected]>
(cherry picked from commit ea8caeb)
Signed-off-by: Nikhil Thomas <[email protected]>
nikhil-thomas added a commit to nikhil-thomas/operator-sdk that referenced this pull request Nov 20, 2019
@nikhil-thomas
Add --watch-namespace to operator-sdk test local sub-command
63e30f9 
This patch resolves mergeconflicts in cherry-pick ea8caeb
and recent refactoring in operator-sdk/pkg/test
operator-framework#2037

Add --watch-namespace flag to `operator-sdk test local <test-dir> --up-local` command

`--deploy-namespace` flag specifies namespace where namespaced resources are deployed for testing

`--watch-namespace` flag specifies the namespace which operator watches during testing. That is the value
of `WATCH_NAMESPACE` envvar

**the behavior of `--watch-namespace` flags is as follows**

when not set
    `WATCH_NAMESPACE` is set to deploy namespace.
    which means when `--watch-namespace` is not set, watch-namespace == deploy-namespace
    operator watched deploy-namespace during testing

`--watch-namespace=foo`
    `WATCH_NAMESPACES=foo` operator watches namespace `foo` during testing

`--watch-namespace=""`
    `WATCH_NAMESPACES=""` operator watches all namespaces during testing

Update tests

Signed-off-by: Nikhil Thomas <[email protected]>
(cherry picked from commit ea8caeb)
Signed-off-by: Nikhil Thomas <[email protected]>
nikhil-thomas added a commit to nikhil-thomas/operator-sdk that referenced this pull request Nov 21, 2019
@nikhil-thomas
Add --watch-namespace to operator-sdk test local sub-command
45621ef 
This patch resolves mergeconflicts in cherry-pick ea8caeb
and recent refactoring in operator-sdk/pkg/test
operator-framework#2037

Add --watch-namespace flag to `operator-sdk test local <test-dir> --up-local` command

`--deploy-namespace` flag specifies namespace where namespaced resources are deployed for testing

`--watch-namespace` flag specifies the namespace which operator watches during testing. That is the value
of `WATCH_NAMESPACE` envvar

**the behavior of `--watch-namespace` flags is as follows**

when not set
    `WATCH_NAMESPACE` is set to deploy namespace.
    which means when `--watch-namespace` is not set, watch-namespace == deploy-namespace
    operator watched deploy-namespace during testing

`--watch-namespace=foo`
    `WATCH_NAMESPACES=foo` operator watches namespace `foo` during testing

`--watch-namespace=""`
    `WATCH_NAMESPACES=""` operator watches all namespaces during testing

Update tests

Signed-off-by: Nikhil Thomas <[email protected]>
(cherry picked from commit ea8caeb)
Signed-off-by: Nikhil Thomas <[email protected]>
nikhil-thomas added a commit to nikhil-thomas/operator-sdk that referenced this pull request Nov 21, 2019
@nikhil-thomas
Add --watch-namespace to operator-sdk test local sub-command
adfe94b 
This patch resolves mergeconflicts in cherry-pick ea8caeb
and recent refactoring in operator-sdk/pkg/test
operator-framework#2037

Add --watch-namespace flag to `operator-sdk test local <test-dir> --up-local` command

`--deploy-namespace` flag specifies namespace where namespaced resources are deployed for testing

`--watch-namespace` flag specifies the namespace which operator watches during testing. That is the value
of `WATCH_NAMESPACE` envvar

**the behavior of `--watch-namespace` flags is as follows**

when not set
    `WATCH_NAMESPACE` is set to deploy namespace.
    which means when `--watch-namespace` is not set, watch-namespace == deploy-namespace
    operator watched deploy-namespace during testing

`--watch-namespace=foo`
    `WATCH_NAMESPACES=foo` operator watches namespace `foo` during testing

`--watch-namespace=""`
    `WATCH_NAMESPACES=""` operator watches all namespaces during testing

Update tests

Signed-off-by: Nikhil Thomas <[email protected]>
(cherry picked from commit ea8caeb)
Signed-off-by: Nikhil Thomas <[email protected]>
nikhil-thomas added a commit to nikhil-thomas/operator-sdk that referenced this pull request Nov 22, 2019
@nikhil-thomas
Add --watch-namespace to operator-sdk test local sub-command
50c496c 
This patch resolves mergeconflicts in cherry-pick ea8caeb
and recent refactoring in operator-sdk/pkg/test
operator-framework#2037

Add --watch-namespace flag to `operator-sdk test local <test-dir> --up-local` command

`--deploy-namespace` flag specifies namespace where namespaced resources are deployed for testing

`--watch-namespace` flag specifies the namespace which operator watches during testing. That is the value
of `WATCH_NAMESPACE` envvar

**the behavior of `--watch-namespace` flags is as follows**

when not set
    `WATCH_NAMESPACE` is set to deploy namespace.
    which means when `--watch-namespace` is not set, watch-namespace == deploy-namespace
    operator watched deploy-namespace during testing

`--watch-namespace=foo`
    `WATCH_NAMESPACES=foo` operator watches namespace `foo` during testing

`--watch-namespace=""`
    `WATCH_NAMESPACES=""` operator watches all namespaces during testing

Update tests

Signed-off-by: Nikhil Thomas <[email protected]>
(cherry picked from commit ea8caeb)
Signed-off-by: Nikhil Thomas <[email protected]>
@joelanford joelanford deleted the e2e-framework branch January 28, 2020 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri approved these changes

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@jmccormick2001 jmccormick2001 Awaiting requested review from jmccormick2001

@hasbro17 hasbro17 Awaiting requested review from hasbro17

@estroz estroz Awaiting requested review from estroz

@theishshah theishshah Awaiting requested review from theishshah

Assignees

@jmrodri jmrodri

@camilamacedo86 camilamacedo86

Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

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