Skip to content

fix: expose the mocked Juju environment variables via os.environ again #1573

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

Conversation

tonyandrewmeyer
Copy link
Collaborator

Rather than only put JUJU_VERSION into os.environ, copy the entire contents of the mock Juju environment variables (and after the run, put the environment dictionary back to the same state it was originally).

In general, we feel that charms should be using ops to access this information, rather than going directly to the raw environment variable, but this was too large a change to make in a refactoring bugfix release, and should wait for an 8.x release (and also ensuring that there are reasonable ways to get everything from ops - for example, the event name is perhaps too tricky to get globally right now).

Fixes #1572

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change looks good to me -- thanks. Two questions:

  1. Have you tested that this fixes the broken charm tests mentioned in #1572? If not, could we double-check that and note it here.
  2. Do you think we should do a point release of ops/ops-scenario after this is merged?

@tonyandrewmeyer
Copy link
Collaborator Author

  1. Have you tested that this fixes the broken charm tests mentioned in ops-testing should expose the mocked JUJU_ environment variables in os.environ #1572?

Yes, although it also needs:

--- a/tests/scenario/test_charm_tracing.py
+++ b/tests/scenario/test_charm_tracing.py
@@ -29,7 +29,7 @@ from charms.tempo_coordinator_k8s.v0.tracing import (
 from ops import EventBase, EventSource, Framework
 from ops.charm import CharmBase, CharmEvents
 from scenario import Context, Relation, State
-from scenario.runtime import UncaughtCharmError
+from scenario.errors import UncaughtCharmError
 
 logger = logging.getLogger(__name__)
 

The only other charm that I have locally that needs this is tempo-k8s-operator (which is archived). We could re-export things from _runtime to runtime but I feel like that would somewhat negate the point of moving everything to the private module anyway. You can get UncaughtCharmError from scenario.errors in 7.0.0, the recent change is not being able to also get it from scenario.runtime. My preference here would be to keep things as they are.

  1. Do you think we should do a point release of ops/ops-scenario after this is merged?

I feel this ties into the general "do we wait for the end of the month" discussion. This doesn't seem as urgent as breaking deferring in some cases - it's only testing, and it's only this one lib as far as we know - but I'm also not opposed to releasing fixes as we get to them.

@benhoyt
Copy link
Collaborator

benhoyt commented Feb 13, 2025

@tonyandrewmeyer Regarding whether we ship a new version, it's debatable, but I'd say this falls into "regression" territory and so we should ship a new (patch?) version.

The scenario.errors vs scenario.runtime change, I agree with you there. That would be an easy one for them to fix -- or just avoid -- in their tests (the environment one less so).

@tonyandrewmeyer
Copy link
Collaborator Author

@tonyandrewmeyer Regarding whether we ship a new version, it's debatable, but I'd say this falls into "regression" territory and so we should ship a new (patch?) version.

Ok. At the moment the only changes in main are docs so I can do this without needing to create a separate branch.

@tonyandrewmeyer tonyandrewmeyer merged commit 101997e into canonical:main Feb 13, 2025
32 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the scenario-put-env-in-os-environ-again branch February 13, 2025 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ops-testing should expose the mocked JUJU_ environment variables in os.environ
3 participants