-
Notifications
You must be signed in to change notification settings - Fork 102
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: new wait for idle #1245
feat: new wait for idle #1245
Conversation
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.
Thanks, it probably seems silly but this is significantly easier for me to follow. check
and Loop.next
look good to me.
I've commented on your FIXME
note -- also a minor refactor suggestion to put the the error checking loops behind a raise_on_error
flag (and likewise for blocked).
Also a few nitpicks on the tests just to do my due diligence, but I'm guessing you're fine with things as is. The most significant note is that it would probably be worth duplicating the check
tests for blocked/errors to cover the "raise_on_{blocked,error}" = False
cases (unless you've already covered that and I've just missed it).
I notice there are a couple of failing integration tests (they all passed last run in #1219) -- related to this refactor, or just more test flakiness? |
failed (quaranteed) integration test:
and stable integration test:
looks like charmhub had a glitch, restarted. |
@james-garner-canonical I've implemented most suggestions, pelase review :) |
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.
Looks great! Real charm CI runs are reassuring too.
Interesting how this charm tests run has two timeouts with old wait for idle in test_upgrade_from_edge
(3.4/stable on focal and jammy) -- which both passed with new wait for idle. Since it's a timeout I guess this could be a flaky test generally, but I do wonder if this indicates something -- was old wait for idle more eager to bail out with a timeout error rather than check everything? I don't think this necessarily needs further investigation right now, just noting it
/merge |
Same as #1219 but using dumb classes instead of async generator.