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: Application.local_refresh method signature #1114

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

dimaqq
Copy link
Contributor

@dimaqq dimaqq commented Sep 27, 2024

Fixes #1100

Closes #881

Charmers already pass a Path to refresh() in integration tests.
This PR standardises that.

Making arguments to local_refresh() explicit, as charmer don't get the default values.

Existing integration test is updated.

Jira: https://warthogs.atlassian.net/browse/CHARMTECH-309

juju/application.py Outdated Show resolved Hide resolved
juju/application.py Outdated Show resolved Hide resolved
juju/application.py Outdated Show resolved Hide resolved
@dimaqq
Copy link
Contributor Author

dimaqq commented Sep 27, 2024

squashed and rebased to pass commit lint

Copy link
Contributor

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. You might need to (re?)sign your commits to merge.

Copy link
Contributor

@Aflynn50 Aflynn50 left a comment

Choose a reason for hiding this comment

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

Looks good, could you include the integration test steps in the PR description? Either just which of the pylibjuju integration tests were run, or if local refresh is not used in there, just a code snippet that tests the functionality .

juju/application.py Show resolved Hide resolved
@dimaqq dimaqq marked this pull request as draft September 30, 2024 00:14
@dimaqq dimaqq force-pushed the fix-local-refresh branch from 7b1650e to 305b41a Compare October 8, 2024 05:40
@dimaqq dimaqq force-pushed the fix-local-refresh branch from 305b41a to be8b2cb Compare October 9, 2024 08:35
@dimaqq dimaqq marked this pull request as ready for review October 9, 2024 08:35
@dimaqq
Copy link
Contributor Author

dimaqq commented Oct 9, 2024

/merge

@jujubot jujubot merged commit 1f5d361 into juju:main Oct 9, 2024
11 of 13 checks passed
@dimaqq dimaqq deleted the fix-local-refresh branch October 10, 2024 00:16
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.

Fix function signature for local_refresh [Bug]: Refreshing a charm with local resources doesn't work
5 participants