-
Notifications
You must be signed in to change notification settings - Fork 344
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
Update tests to work on OpenShift #323
Conversation
Signed-off-by: Kevin Earls <[email protected]>
@pavolloffay @objectiser @jpkrohling Please review |
Codecov Report
@@ Coverage Diff @@
## master #323 +/- ##
=======================================
Coverage 88.94% 88.94%
=======================================
Files 69 69
Lines 3112 3112
=======================================
Hits 2768 2768
Misses 234 234
Partials 110 110 Continue to review full report at Codecov.
|
test/e2e/daemonset.go
Outdated
req, err := http.NewRequest(http.MethodGet, url, nil) | ||
if err != nil { | ||
return err | ||
func getJaegerDefinition(namespace string, name string) *v1.Jaeger { |
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.
Think this needs to be renamed to indicate it is returning a AgentAsDaemonSet definition.
test/e2e/daemonset_with_ingress.go
Outdated
@@ -0,0 +1,111 @@ | |||
package e2e |
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.
Testing the ingress should be independent of the agent, whether deployed daemonset or sidecar. Might be better to have variants of the all-in-one-test.go
and production-test.go
that use ingress instead of port-forward?
test/e2e/all_in_one_test.go
Outdated
defer portForward.Close() | ||
defer close(closeChan) | ||
|
||
|
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.
nit: remove extra line
test/e2e/daemonset.go
Outdated
}) | ||
} | ||
|
||
|
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.
nit: remove two extra lines
Routes can be queried with the same API as other objects
You can use this PR to see how we know that we are running on OCP or k8s #217 |
Signed-off-by: Kevin Earls <[email protected]>
HI @objectiser I've updated this based on your previous comments. Could you please take another look? |
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 - although did you checkout the link provided by Pavol regarding routes? This could be handled in a separate PR I guess, if you wanted to get this one merged.
@objectiser I'm looking at that now. If possible though I'd like to get this merged and add that as a separate issue. |
@kevinearls that is fine with me. |
@objectiser Thanks. Can you merge it then, or do I need review from @pavolloffay and @jpkrohling too? |
This is the second set of changes for #227. I've done the following