fix: remove ops.main.main deprecation warning, and avoid warnings in action output #1496
Conversation
Thinking on this more, I think it should be the other one. I'll switch over (and fix whatever the test issue is - maybe 3.8 compatibility since they passed locally) and un-draft. No need to review until then. |
I think there's a 3rd option: remove the warning entirely, and solve the ops.main.main -> ops.main some other way:
Let's brainstorm at the next standup, perhaps? |
I did think about this too. This one in particular is complicated because it's "outside" the charm, so it doesn't get picked up in unit tests and is also complicated to code around because the framework isn't set up yet. I do think that we should have a PR that prevents the general issue of DeprecationWarnings that caused by code in src/charm.py ending up in action output. But we could potentially do that and also give up on this specific warning in favour of other approaches. |
|
Decision from Charm Tech is to remove the warning altogether to keep it simple. And then Charm Tech (whoever works on this) can open 2 or 3 PRs jubilantly (!) to fix this in the "culprit teams". |
tonyandrewmeyer
force-pushed
the
cleaner-warnings-1460
branch
from
2c0d187 to
a589f26
Compare
Yoikes: |
tonyandrewmeyer
changed the title
Ouch. We could still let 2 or 3 teams know with PRs, but yeah, with those numbers, maybe that's not a good approach. I guess just a polite message in Charm Development and leave it at that? Out of interest, are there 312 charms in your dir now, or does this occur more than once per charm? |
More than once. Some are documentation, but that really ought to get fixed too. Some are in libs, so there's some duplication that would remove the number of PRs, but be more annoying to make sure the change is in the right PR. Some repos also have quite a few charms in them, which ups the volume. |
| """Direct the warning to Juju's debug-log, and don't include the code.""" | ||
| logger.warning('%s:%s: %s: %s', filename, lineno, category.__name__, message) | ||
|
|
||
| warnings.showwarning = custom_showwarning |
There was a problem hiding this comment.
Would this suppress warnings from third-party libraries in charmers' tests?
It might still be beneficial for actions, but could risk charmers overlooking issues in other libraries they use.
There was a problem hiding this comment.
Would this suppress warnings from third-party libraries in charmers' tests?
This doesn't really suppress the warnings, it just changes them to go through the logging system (I suppose if your log level is higher than warning then they are effectively suppressed, but I would argue that you have asked for that to happen).
This would apply to any warnings including third-party libraries, yes. However, I think that's the behaviour we want - if some third-party library generated a warning during an action, then I think that should end up in the Juju debug-log (or Loki or anywhere else people have directed the logging to) and should not end up in the action output.
This wouldn't happen with charm unit tests, because we (at least in Scenario) have a custom log/warning setup there - we don't do the special-casing for actions, and the debug-log entries end up in a side-effect collection in the context object. It would happen with integration tests but I'd argue that's what we want: the same behaviour as when the charm is run in production. I don't think people notice warnings in integration tests, but I think they often do in unit tests, so that's where it's important they show up.
If charms run their integration tests so that the charm is run with -W error (last time I looked, no-one does this, but I think it would be a good idea) then it does still apply (I manually checked this case when creating the PR), so the charm would crash, failing the test.
Background:
warnings.warncall outputs to stderr by default.main()other thanimport ops ... ops.main()__main__module.main()or execute thecharm.pyfile, so they will not trigger this warning. That means that we're relying on integration tests.The above means that, currently, the deprecation warning is included in the action output - this warning is intended for the developer of the charm, not the user of the charm, so we want to change this.
Firstly, the
ops.main.maindeprecation warning is removed. It's too awkward to have a warning that gets emitted before the framework is set up. Instead, we'll rely oncharmcraft analyseand static checks.However, while that will solve the specific issue in #1460, we do also want to avoid any future leakage of warnings into action output if there are other warnings emitted that originate in the
__main__model.To that, rather than rely on Juju capturing the warning via stderr, we use the logging package to capture all warnings and redirect them to logging calls. These are then directed to Juju's debug-log via the existing mechanism. While we are doing this, we minimise the output to include references to the filename and line, but not the actual code itself (which may be sensitive).
When running (Scenario) tests, we ensure that the warning and logging systems use their default behaviour, so that pytest and other tools can work as expected.
Fixes #1460