Skip to content

docs: Improve Container.pull docs to avoid leaky files#2508

Merged
tonyandrewmeyer merged 8 commits into
canonical:mainfrom
tonyandrewmeyer:rainy/2504-container-pull-context-manager-docs
May 27, 2026
Merged

docs: Improve Container.pull docs to avoid leaky files#2508
tonyandrewmeyer merged 8 commits into
canonical:mainfrom
tonyandrewmeyer:rainy/2504-container-pull-context-manager-docs

Conversation

@tonyandrewmeyer
Copy link
Copy Markdown
Collaborator

@tonyandrewmeyer tonyandrewmeyer commented May 26, 2026

Container.pull (and the Client.pull it wraps) return an opened file that is never explicitly closed. This leaks a resource until garbage collection (which then emits a warning), and in the case of encoding=None can leak that resource until process end. This is not a significant problem, but we should document the clean, rather than leaky, way to use these methods.

The how-to guide has poor suggestions as well, but those are all being replaced in #2470 (to use pathops) so are not worth fixing here.

Fixes #2504

Comment thread docs/howto/manage-containers/manage-files-in-the-workload-container.md Outdated
Comment thread ops/model.py Outdated
Co-authored-by: Tony Meyer <tony.meyer@gmail.com>
@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review May 26, 2026 21:24
@tonyandrewmeyer
Copy link
Copy Markdown
Collaborator Author

@dwilding and @james-garner-canonical: in the issue, James suggested using pathops, which handles pull correctly. I'm not sure how much we are using pathops within our docs - I imagine not in reference docs, but maybe in the how-to guides? If we would rather adapt the bits of doc to use that approach, I'm happy to do that.

Comment thread docs/howto/manage-containers/manage-files-in-the-workload-container.md Outdated
Copy link
Copy Markdown
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.

The changes to the docstrings and the custom notices example are a nice fix, thanks!

The changes to the managing files in the workload container guide clash with #2470. I'd suggest dropping the changes to the file made in this PR, and adding any that are relevant to #2470 (though I'm not sure if any of them are since we're switching to pathops for most examples -- but maybe worth mentioning the file closing issue doesn't affect pathops?).

Other suggestions are all non-blocking.

Comment thread ops/model.py Outdated
Comment thread ops/model.py Outdated
Comment thread ops/pebble.py Outdated
Comment thread ops/pebble.py Outdated
@tonyandrewmeyer
Copy link
Copy Markdown
Collaborator Author

The changes to the managing files in the workload container guide clash with #2470.

Ah yes, the PR I keep forgetting that I have only half-reviewed. Fixed now! Thanks for calling that out.

Comment thread docs/howto/manage-containers/manage-files-in-the-workload-container.md Outdated
Comment thread docs/howto/manage-containers/manage-files-in-the-workload-container.md Outdated
Comment thread docs/howto/manage-containers/manage-files-in-the-workload-container.md Outdated
Co-authored-by: Tony Meyer <tony.meyer@gmail.com>
Comment thread docs/howto/manage-containers/manage-pebble-custom-notices.md Outdated
Comment thread ops/pebble.py Outdated
Co-authored-by: James Garner <james.garner@canonical.com>
Comment thread ops/pebble.py Outdated
@tonyandrewmeyer tonyandrewmeyer merged commit 1b2c6e8 into canonical:main May 27, 2026
59 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the rainy/2504-container-pull-context-manager-docs branch May 27, 2026 23:42
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.

Improve Container.pull docs to avoid leaky files

3 participants