-
Notifications
You must be signed in to change notification settings - Fork 92
feat(templates): add workload module to Kubernetes and machine profiles #2242
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(templates): add workload module to Kubernetes and machine profiles #2242
Conversation
|
Adding @mr-cal on this since he made the upstream version. Callahan, how hard do you think it'd be to do this in craft-application? Maybe we could make the template filenames format strings that take a small dict? |
lengau
left a comment
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 PR David! Approving the template content themselves - I want to discuss the init changes and the docs with their relevant SMEs though before full approval.
mr-cal
left a comment
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.
@lengau, it's certainly possible to extend the existing design to allow for this. This is the second scenario where we have template-specific info. The other scenario is providing unique documentation links for each template.
I've been thinking about where this information could live. My current idea is a dict that we pass to the service, which maps template names to template data. That data could include a callables, for scenarios like this where the file renaming is complex.
Approving because the PR looks good and it can go upstream with some integration work.
medubelko
left a comment
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 @dwilding! I left some suggestions for a bit of doc refinement, but overall these look fine to me.
|
Thanks all for the reviews! @medubelko, I'll work on the doc changes first thing next week. |
medubelko
left a comment
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.
Latest doc change LGTM.
lengau
left a comment
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!
This PR update the
kubernetesandmachineprofiles to include a standalone module for workload-specific logic. Other profiles are not changed in any way.In addition to the new workload module, the updated profiles have a bit more structure in
charm.pyto illustrate the basic flow of logic through the charm & workload module, and improved unit tests intest_charm.py.This PR includes
kubernetesandmachineprofiles.charmcraft/application/commands/init.py, to determine the name of the workload module from the charm name (usingworkload.pyif the charm has a generic name such ascharmork8s-operator).tests/integration/commands/test_init.py, to test the updated functionality (with tests passing).charmcraft init, to make users aware of the workload module.src/<workload>.pyfile (new)src/charm.pyfile (updated)Hopefully it will be easy to follow my commits in sequence. I tried to make the changes in a logical step-by-step order.