Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vveliev-tc
Copy link

@vveliev-tc vveliev-tc commented Nov 1, 2022

PR progress checklist (to be filled in by reviewers)

  • Changes to documentation are appropriate (or tick if not required)
  • Changes to tests are appropriate (or tick if not required)
  • Reviews completed

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 tests

Does 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

  • Updated the README (e.g. Available states).
  • Updated pillar.example.

Testing checklist

  • Included in Kitchen (i.e. under state_top).
  • Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
  • Updated the relevant test pillar.

Additional context

@vveliev-tc vveliev-tc requested review from a team as code owners November 1, 2022 23:28
Copy link
Member

@daks daks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, but I would prefer a more 'agnostic' name like libs which could be used for other type of libraries if needed later.

Also, we must be sure it has no negative impact on unusual setups (like salt-ssh for example). ping @baby-gnu @myii

@vveliev-tc
Copy link
Author

vveliev-tc commented Nov 8, 2022

I was actually not sure about the name for the folder, since there is a git ignore for lib https://github.com/saltstack-formulas/template-formula/blob/master/.gitignore#L18

should it be libs/jinja/map.jinja or just libs/map.jinja ?

@baby-gnu
Copy link
Contributor

baby-gnu commented Nov 8, 2022

I was actually not sure about the name for the folder, since there is a git ignore for lib https://github.com/saltstack-formulas/template-formula/blob/master/.gitignore#L18

should it be libs/jinja/map.jinja or just libs/map.jinja ?

I personally prefer libs/map.jinja.

Regards.

@daks
Copy link
Member

daks commented Nov 15, 2022

If we all three agree for /libs directory, let's go for it! :)

@dafyddj
Copy link
Contributor

dafyddj commented Nov 18, 2022

This might be a minor detail, but I would go for lib myself. The fact that lib is in .gitignore seems to be by accident rather than by design.

@vveliev-tc
Copy link
Author

vveliev-tc commented Nov 18, 2022

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
jinja - indicates that it's jinja files

I have no strong opinion on any of them as long as its not in formula root directory

@baby-gnu
Copy link
Contributor

baby-gnu commented Dec 6, 2022

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 jinja - indicates that it's jinja files

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 libs/ directory, hadoop-formula.

This formula is not updated since 2018 so I really advocate to don't bother and use libs/ ;-)

Regards.

Copy link
Member

@daks daks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

@baby-gnu
Copy link
Contributor

Thanks :)

Now, I'm wondering how to translate this to ssf-formula 😅

@baby-gnu
Copy link
Contributor

Hello, looking at this for another formula, I wonder if the per formula post-map.jinja shouldn't be under parameters/ instead of libs/.

This post-map.jinja is more about specific data manipulations instead of a generic library.

As a user of a formula, I'll give more attention to files under parameters/ to understand a formula rather than under libs/.

Only 2 formulas use this post-map.jinja for now:

@daks
Copy link
Member

daks commented Dec 22, 2022

Thanks :)

Now, I'm wondering how to translate this to ssf-formula sweat_smile

I have no idea neither.
This is a problem because I think only Imran knows how it works

@vveliev-tc
Copy link
Author

Hello, looking at this for another formula, I wonder if the per formula post-map.jinja shouldn't be under parameters/ instead of libs/.

This post-map.jinja is more about specific data manipulations instead of a generic library.

As a user of a formula, I'll give more attention to files under parameters/ to understand a formula rather than under libs/.

Only 2 formulas use this post-map.jinja for now:

I'm not sure to be honest sounds like parameters/ will be better place for post-map.jinja as jinja files will be expected under parameters as well.

does it need to be resolved for this PR?

@baby-gnu
Copy link
Contributor

baby-gnu commented Jan 4, 2023

I'm not sure to be honest sounds like parameters/ will be better place for post-map.jinja as jinja files will be expected under parameters as well.

I'm not sure to understand your point 🤔

To me:

  • libs/ is a good place for code reusable between formulas, which could easily be managed by automation tools like ssf-formula
  • post-map.jinja is not something generic but much more formula dependent and, to me, it makes sense to put it somewhere else

does it need to be resolved for this PR?

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.

@vveliev-tc
Copy link
Author

@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?

@vveliev-tc vveliev-tc force-pushed the master branch 2 times, most recently from ffca1d6 to 4513d39 Compare January 4, 2023 23:08
Copy link
Contributor

@baby-gnu baby-gnu left a 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.

TEMPLATE/libs/map.jinja Outdated Show resolved Hide resolved
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.

4 participants