-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Introduce a template system for extending SaltStack open #34921
Conversation
weirdly the linter is raising an issue with the logging import about a Py3 compat issue. I develop on py3 and it works fine! |
And the other one is dumb "Unnecessary parens after u'print' keyword", well yeah but that's for Python 2, I wouldn't want to write this purely for Py2 |
from __future__ import print_function That will take care of the print function lint issue. |
The other lint issue is not about logging, if you look at the console output on Jenkins it will really tell you want its missing for py3 compat. from salt.ext.six.moves import zip |
@tonybaloney I really like this concept, I am a little worried about adding another dep in cookiecutter, so I will need to chew on this for a bit. |
@tonybaloney we REALLY like this, but as we discuss it internally, we are leaning towards saying that this should be a standalone project instead of being part of Salt itself. We are still debating this but I thought I would ask for your feedback on the subject. |
|
||
log = logging.getLogger(__name__) | ||
|
||
__virtualname__ = '{{module_name}}' |
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.
The __virtualname__
does not need to be the module name although it often is, perhaps add support for a configurable virtualname
parameter defaulting to module_name
?
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.
slightly long-winded answer, but I added a dictionary of questions in the template, the default value is also templated-using jinja. d99b21c
And that's about it for now. I hope I'm not sounding too picky. |
@s0undt3ch not at all, I'll work through those today. Salt practically has its own standard library so it's best I ask the experts. I looked at salt.version and couldn't figure it out so that all makes sense |
…platise the defaults
OK! I'm done. Docs, tests, templates (I ended up adding user-defined questions to the template context as well). Pls review @thatch45 @cachedout |
linter is green, the test failures don't have anything to do with these changes *famous last words |
:returns: Details about the template | ||
:rtype: ``tuple`` | ||
''' | ||
with fopen(path, "r") as template_f: |
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.
salt.utils.fopen
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.
@cachedout I dropped the namespace on the import https://github.com/saltstack/salt/pull/34921/files#diff-585fafce47a7c44a935dd239a8b93ff0R26
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.
Ah! FWIW, our style guide suggests using fully-qualified names. (Mostly because I am easily confused.) ;]
I think we are likely good to merge here.... |
''' | ||
from __future__ import absolute_import | ||
from __future__ import print_function | ||
import yaml |
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.
It might be nice to use the salt serializer classes here. We do some work to try to massage/optimize YAML. See salt.serializers.yaml
for example.
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.
ok done 39159cc
so its just waiting on a response around a jinja class and then I think we're finally good to merge? |
Merged! Thanks so much @tonybaloney. This is really excellent. Many people will benefit from your hard work here! |
sweeeeet! Thanks everyone, I actually learnt a lot of new stuff doing this PR. |
This gets the Awesome label too. Thanks for putting up with our scrutiny, it is not usually this bad! We will also need to make sure that this is covered in the release notes for Carbon |
What does this PR do?
Extending salt by adding new modules, states, adding tests, is quite tricky because there's not really a good example to start from. It's a job of copying and pasting from another module, the salt team don't get much control over this and there isn't much consistency.
What I've done is:
salt-extend
, which asks the user a series of questions about what they're doingsalt.utils.extend
which wraps thecookiecutter
package to generate a new project and merges it into the salt source codeCommand line usage for
salt-extend
is this (anything not sent on command line will be prompted)If the user (like a core saltstack developer) does work often, they can save their details in the ~/.cookiecutterrc file https://cookiecutter.readthedocs.io/en/latest/advanced/user_config.html
I've submitted 4 templates as an example, based on how I typically extend Salt functions.
module
)state
)You can use this to provide more templates to any of the SaltStack ecosystem, those are the only 4 that I'm familiar with. The templates use Jinja2 so you can do all sorts of crazy stuff like
Please review Salt's Contributing Guide for best practices.
Comments please, I'm no way tied to the design of this or having
templates
in the project root for example, so I've tagged all the top contributors for feedback :-)CC @techhat @cachedout @rallytime @thatch45 @s0undt3ch @terminalmage @nmadhok