-
Notifications
You must be signed in to change notification settings - Fork 85
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
refactor: moving jinja to separate dir for cleaner layout #252
base: master
Are you sure you want to change the base?
Conversation
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.
I was actually not sure about the name for the folder, since there is a git ignore for should it be |
I personally prefer Regards. |
If we all three agree for |
This might be a minor detail, but I would go for |
let agree on the style before I change it again :). lib/s - maybe confused with formula libs IMO. altho is usually under package/install for formulas I have no strong opinion on any of them as long as its not in formula root directory |
Actually, there is only a single formula with a This formula is not updated since 2018 so I really advocate to don't bother and use Regards. |
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 :)
Now, I'm wondering how to translate this to |
Hello, looking at this for another formula, I wonder if the per formula This As a user of a formula, I'll give more attention to files under Only 2 formulas use this |
I have no idea neither. |
I'm not sure to be honest sounds like does it need to be resolved for this PR? |
I'm not sure to understand your point 🤔 To me:
This PR will be a starting point for new standard accross formulas, I think we should take time to make it clean and permit to everybody to give their points. Regards. |
@baby-gnu I think we are on the same page, I have updated map.jinja to reflect that. Is there anythinb else I should change? |
ffca1d6
to
4513d39
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.
This is a good PR, my proposal of change is completely optional since it's just me being very picky.
PR progress checklist (to be filled in by reviewers)
What type of PR is this?
Primary type
[build]
Changes related to the build system[chore]
Changes to the build process or auxiliary tools and libraries such as documentation generation[ci]
Changes to the continuous integration configuration[feat]
A new feature[fix]
A bug fix[perf]
A code change that improves performance[refactor]
A code change that neither fixes a bug nor adds a feature[revert]
A change used to revert a previous commit[style]
Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)Secondary type
[docs]
Documentation changes[test]
Adding missing or correcting existing testsDoes this PR introduce a
BREAKING CHANGE
?No.
Related issues and/or pull requests
Describe the changes you're proposing
Simply moving jinja files to separate folder to make bit more cleaner root folder. (jinja may scare new contributors ;) )
Pillar / config required to test the proposed changes
Debug log showing how the proposed changes work
Documentation checklist
README
(e.g.Available states
).pillar.example
.Testing checklist
state_top
).Additional context