-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Golang E2E in Openshift API-CI #1384
Conversation
cb1f293
to
dedea01
Compare
This has been fully retested and is ready to be reviewed again. Corresponding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -47,7 +47,7 @@ func Become(ctx context.Context, lockName string) error { | |||
|
|||
ns, err := k8sutil.GetOperatorNamespace() | |||
if err != nil { | |||
if err == k8sutil.ErrNoNamespace { | |||
if err == k8sutil.ErrNoNamespace || err == k8sutil.ErrRunLocal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question: At this point is there ever a case where err == k8sutil.ErrNoNamespace
and err != k8sutil.ErrRunLocal
?
Looking at the errors returned by cluster specific functions such as initOperatorService()
and GetOperatorNamespace()
, GetOperatorName()
it seems they will all return ErrRunLocal
if running locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a user is using our commands, ErrNoNamespace
probably shouldn't be hit since we manually force the run mode. I do assume it's possible if a user manually runs it themselves with go run cmd/manager/main.go
.
/hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion on refactoring then LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lgtm, couple of questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
New changes are detected. LGTM label has been removed. |
/retest |
1 similar comment
/retest |
This is currently a WIP for a POC to allow us to run the entire golang E2E suite in openshift-ci with the exception of the image building part of
operator-sdk build
, which can be replaced with comprehensive unit tests.