-
Notifications
You must be signed in to change notification settings - Fork 880
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
feat: Support URI sources in write_files
module
#5505
feat: Support URI sources in write_files
module
#5505
Conversation
As I continue to work on these changes, I have a few questions for anyone with real cloud-init experience:
|
a0513d9
to
791ee42
Compare
This isn't really an appropriate example, since the `write_files` module now supports URL sources. (Also, this example wrote to `/tmp`, which conflicts with other advice on the examples page.)
791ee42
to
44839b6
Compare
...rather than passing a Paths object, which is just pain. Also, this way lets us pass None as well, if desired.
I have |
We should also test reading from a "real" (i.e. HTTP[S]) URI, though setting this up might be... involved. Would we need a local server?
And this is why we test.
a6937c9
to
792a58e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution! Overall, things look good here. I left a few inline comments as well as answers to your questions here.
Do we want to support templating of the source URI?
cloud-config supports jinja templating, so module specific templating shouldn't be necessary (if I understood the question correctly). The cc_phone_home snippet you found is very old and likely predates the ability to use jinja.
Which, if any, of the other kwargs supported by readurl should be exposed as module parameters?
That's a great question with no great answer. I prefer to be opinionated as to remove the number of options that need to be specified. The retry defaults you provided seem reasonable, though I'd throw a comment in specifying that they're completely arbitrary and just seemed like good defaults. It makes sense to include headers for any kind of token or basic auth, but I don't think we need more options than that. To accomodate this, I think it'd be better to make source
a dictionary. It can then have subkeys like uri
which would be required and headers
that would be optional. If in the future we decide we need more than that, we can then add them to the dictionary.
I have one (now two) new tests currently, which access content from a file:// URI. There should probably also be one for a "real" HTTP(S) URI as well, but I'm not sure about setting that up — would we also need to launch a lightweight HTTP server? Is there already some kind of support for this in the testing framework?
For unit tests, we use responses to mock the server responses. For this effort I think that should be sufficient. Can we also get a test for the fallback behavior?
Let me know if you have any extra questions!
Thanks for the feedback — that's very useful, especially since I'm new to cloud-init. Especially the " (Also, side note: there's already a test for the fallback behavior; that's the second one I alluded to in my previous edit. It currently uses a file URI, but that could change if desired.) |
Ah, sorry, I missed that. That works! |
In response to PR feedback.
i.e. if the URL fails and no `content` was provided, don't even create a file.
This really made the linting system mad. Oops.
This is really important with such a large codebase, isn't it?
All right, comments addressed — and a fair bit of formatting fixed; tests under older Pythons should actually pass now! FWIW, I did try changing the fallback test to use an HTTP URI, but that takes several seconds to fail (due to going through the OS's networking stack), so I've left it with a file URI. Not worth slowing down the tests for a tiny bit of added realism. |
ba739c9
to
1c55f6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! I left a few more minor inline comments, but I think it's really close now. I'm also approving the CI runs, so check for any failures there too.
Great, thanks! I've been running tests manually on my system as well, but it looks like I missed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LRitzdorf Thanks for this! I see a small change that we should make to the schema before merging. See my comments below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me. Thanks @LRitzdorf, and welcome to cloud-init!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the contribution @LRitzdorf !
This change adds an optional `source` key to the `write_files` module, allowing users to specify a URI from which to load file contents. This facilitates more flexible multi-part configurations, as file contents can be managed via external sources such as independent Git repositories. Fixes canonicalGH-5500
This change adds an optional `source` key to the `write_files` module, allowing users to specify a URI from which to load file contents. This facilitates more flexible multi-part configurations, as file contents can be managed via external sources such as independent Git repositories. Fixes GH-5500
Proposed Commit Message
Additional Context
Resolves #5500
Test Steps
Unit tests, included, cover:
file://
and HTTP URI functionalitycontent
if the provided URI fails to return usable data)Merge type