-
Notifications
You must be signed in to change notification settings - Fork 46
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
feat: Enabled virtual-circuit sub commands in metal-cli #369
Conversation
1e23c8e
to
018cc34
Compare
018cc34
to
dfc8b69
Compare
dfc8b69
to
2f0f46f
Compare
749fee1
to
d37183a
Compare
d37183a
to
12a370d
Compare
12a370d
to
d4b5946
Compare
|
d4b5946
to
382bb97
Compare
The commands in your example output look like a good basis for e2e tests on the new subcommands. Could you add those? |
382bb97
to
8786b6b
Compare
a8cc1eb
to
92ba43b
Compare
92ba43b
to
e063e14
Compare
e063e14
to
b0a4acd
Compare
|
b0a4acd
to
2dbf29a
Compare
2dbf29a
to
810b545
Compare
810b545
to
b7b4ce6
Compare
b7b4ce6
to
2db565e
Compare
2db565e
to
159204b
Compare
159204b
to
a6c5452
Compare
The latest test run failed during cleanup for a test:
A 422 on delete usually indicates that the code is doing something wrong (as opposed to an intermittent platform issue). Do these failures not happen when you run the tests locally? |
A re-run of the tests passed, so the failures do not happen consistently. I'm doing a 3rd run now to get a feel for the failure rate. I'm guessing this is some kind of timing issue: the tests pass if the platform happens to handle the cleanup requests quickly enough, but if there's a delay somewhere in the deletion process, the tests fail. If that's the case we can merge as-is and shore up the tests in a separate PR. Ideally that separate PR wouldn't come in until after all the other in-flight PRs are merged, but if the intermittent failures are causing too many problems for review we may need to fix them sooner. |
Not observed in local run |
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.
Tests added in this PR passed 2 out of 3 times in CI, so this can go in as-is and we can improve the resilience of the tests later.
Part of #307