-
Notifications
You must be signed in to change notification settings - Fork 25
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
Implemented Azure e2e tests #352
Conversation
445cfe9
to
ffaeb8e
Compare
Looking at the latest test failure, my suspicion is the names aren't unique for the cluster names for each provider test and they're running over each other. It might be worth appending a provider name to the end of the |
84d25d0
to
a8474a7
Compare
e3853f2
to
e88e451
Compare
9bed4db
to
fcb384f
Compare
fcb384f
to
686b13f
Compare
@kylewuolle In the future can use try not to use force-pushes? At least not after someone has initially reviewed your code. It makes reviewing your code changes after a big initial review harder, you can always just squash the ending result into a single commit. |
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, but a couple of my prior comments are still unresolved, either unchanged or no comment was added to them.
test/managedcluster/azure/azure.go
Outdated
} | ||
|
||
spec, found, err := unstructured.NestedMap(list.Items[0].Object, "spec") | ||
if !found || err != nil { |
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.
The !found || err != nil
logic here should be changed, perhaps this was missed in the last round of changes.
Not sure if getAzureInfo
can be a GinkgoHelper as well, it doesn't look like it runs in an Eventually
.
test/managedcluster/azure/azure.go
Outdated
yamlDoc, err := yamlReader.Read() | ||
|
||
if err != nil { | ||
if err == io.EOF { |
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.
Using errors.Is
is the better idea here because errors.Is
supports wrapped errors while err ==
will not.
@kylewuolle please squash commits like "fix linting error" and let's verify azure part is green before we land it |
@kylewuolle if you cannot reproduce the latest failure locally -- you can use try to use the tmate action: https://github.com/mxschmitt/action-tmate to live debug what's going on with the My best guess is something is going wacky with the credential creation part, as the CAPA provider won't start if the We might want to modify the e2e test to run |
d201d5e
to
f0d08ff
Compare
393876f
to
889bff7
Compare
889bff7
to
dab4275
Compare
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.
As discussed, go ahead and merge what you currently have @kylewuolle and I'm going to work to get the E2E tests all green in #360
Implement Azure template e2e tests. Closes #210.