Skip to content
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

fix(model): make Secret.set_content invalidate local cache #1001

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

benhoyt
Copy link
Collaborator

@benhoyt benhoyt commented Aug 30, 2023

This is so that if get_content() is called after, it fetches and returns the expected new content. We could also set self._content to the new content, but re-fetching it from Juju seems better.

This is so that if get_content() is called after, it fetches and
returns the expected new content. We could also set self._content to
the new content, but re-fetching it from Juju seems better.
old_content = secret.get_content()
self.assertEqual(old_content, {'foo': 'bar'})
secret.set_content({'new': 'content'})
fake_script(self, 'secret-get', """echo '{"new": "content"}'""")
Copy link
Member

Choose a reason for hiding this comment

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

I wish we didn't have to force the fake_script, that certainly makes the test feel like a tautology (though I realize it actually isn't).

Also, should we be adding a test around testing.Harness and that when we're using its backend that we get the right behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the second fake_script felt weird, but it's correct here. I thought about have a more realistic secret-set/secret-get that wrote to and read from a file instead, but that seems like unnecessary work for what we're testing here. Here we just need to assert that the second get_content() call returns the updated content rather than the cached content as it did previously.

Regarding Harness, I think that is an interesting idea in general, but I don't think we'd be adding anything for this case by adding a specific test. This isn't a Harness problem but a model.Secret problem -- it's backend-independent.

@benhoyt benhoyt merged commit b6ffaf0 into canonical:main Aug 30, 2023
17 checks passed
@benhoyt benhoyt deleted the secret-set_content-invalidate-cache branch August 30, 2023 02:15
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.

3 participants