fix(convert-formulas.sh): add -_ to allowed chars in formula name #207

dafyddj · 2020-09-29T19:48:37Z

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

Allow '-' and '_' in formula names. Formulas already exist with '-' in the name (prior to -formula),
so it seems reasonable to allow creation of new ones.

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

pull-assistant · 2020-09-29T19:48:39Z

Best reviewed: commit by commit

Optimal code review plan

fix(convert-formulas.sh): add -_ to allowed chars in formula name

Powered by Pull Assistant. Last update b334c82 ... b334c82. Read the comment docs.

vutny

Looks good

baby-gnu · 2020-09-30T07:20:47Z

I'm wondering if the tooling accept to have a - in the name of the formula. I think @myii told me something about this.

noelmcloughlin · 2020-09-30T08:39:53Z

I would be against allowing underscore in formula names because it invalidates current assumptions around semantic naming. Some core maintainers (i.e. me) expect a standard semantic name of <component>-formula using 'dash' as the historic community convention for formula name. In https://github.com/saltstack-formulas/salter I assume '-' and can add '_' but increased flexibility in the naming convention introduces complexity in other places.

dafyddj · 2020-09-30T12:55:07Z

I'm wondering if the tooling accept to have a - in the name of the formula. I think @myii told me something about this.

There are at least 13 formulas that have - in their names, so I don't believe there is a tooling issue (although I welcome correction on that), and this change just brings the convert-formula.sh into line with current practice.

I add _ for flexibility, but perhaps it is not necessary.

myii · 2020-09-30T13:00:35Z

We've currently got two formulas using an underscore in the name:

myii · 2020-09-30T13:06:10Z

I'm wondering if the tooling accept to have a - in the name of the formula. I think @myii told me something about this.

This is a link to that conversation. We can manage (and pretty much have to, due to the sheer volume of formulas already doing this).

https://freenode.logbot.info/saltstack-formulas/20200325#c3478581-c3478644

- - -

20:54 nebuchadnezzar a question, is there a reason to limit the new name used by ./bin/convert-formula.sh ?

20:54 myii What limit is that? Let me look at it again...

20:54 nebuchadnezzar I would like to create a new formula for go-wisper/go-carbon/go-carbonapi and this limitation is too restrictiv for me

20:55 nebuchadnezzar erf, it's not go-carbonapi it's just carbonapi ;-)

20:55 myii But where will it be located in GitHub (or other hosting service)?

20:56 myii https://;project;/https://;repo; right?

20:56 myii CC: @dafydd -- you may be interested to come back to this little discussion.

20:57 nebuchadnezzar I hope it will be at saltstack-formula/go-wisper-formula

20:59 nebuchadnezzar There are 51 formulas with a dash in their name

20:59 myii OK, maybe the regex is a little restrictive.

20:59 myii But it does make difficulties, though.

21:00 myii Take the syslog-ng-formula for example. I remember there being some horrible things we had to do to get it working in the CI, including changing those dashes to underscores.

21:01 nebuchadnezzar ho, I was not aware of that

21:01 myii https://github.com/myii/ssf-formula/blob/c259684c43e49730445ca7c3327b462447418b25/ssf/formulas.yaml#L4281-L4307

21:01 myii You can see the underscores shown in some of the places there.

21:02 myii Maybe it could have been avoided with better design, I'm not sure. I didn't convert that formula.

21:07 nebuchadnezzar thanks, I'll start locally and I'll change to a better name after, thanks

21:08 myii You're welcome. If you figure out how to get it working with the dash instead of using an underscore, let me know. That will come in useful when converting some of the other formulas.

myii · 2020-09-30T13:23:14Z

@dafyddj I just need to get around testing both additions locally and then we get this merged if all is OK.

Actually, we really should update the test to have a formula name that contains both - and _, i.e. modify this section:

template-formula/.travis.yml

Lines 124 to 135 in e668832

    
           # Test the conversion of `template-formula` into another formula 
        
           # ready for development 
        
           - env: 'Conversion' 
        
             name: 'Test: bin/convert-formula.sh' 
        
             script: 
        
               - git clone . tmp/converted-formula 
        
               - cd tmp/converted-formula 
        
               - DEBUG=true bin/convert-formula.sh converted 
        
               - '[ $(git rev-list HEAD --count) -eq 2 ]' 
        
               # Quick visual check that correct files have been updated 
        
               - git show --pretty="" --name-status 
        
               - bin/kitchen verify default-debian-10-tiamat-py3

How about changing from converted-formula to something like test-the_use-this-template_button-formula?

dafyddj · 2020-09-30T13:57:11Z

I would be against allowing underscore in formula names because it invalidates current assumptions around semantic naming. Some core maintainers (i.e. me) expect a standard semantic name of <component>-formula using 'dash' as the historic community convention for formula name. In https://github.com/saltstack-formulas/salter I assume '-' and can add '_' but increased flexibility in the naming convention introduces complexity in other places.

Are you referring here specifically to the -formula part of the name because I do not expect anyone to use _formula.
Otherwise, I looked at salter (which I wasn't previously aware of) and can't immediately see where the underscore would cause a problem.

dafyddj · 2020-09-30T14:02:13Z

How about changing from converted-formula to something like test-the_use-this-template_button-formula?

Good idea.

baby-gnu · 2020-10-05T13:34:08Z

I would like to propose some formulas we used at work and one of them would be pam-mount-formula.

baby-gnu · 2020-10-07T09:23:20Z

bin/convert-formula.sh

+  tag_out=$(git tag --list | xargs git tag --delete)
+  if [ "${DEBUG:-false}" = "true" ]; then
+    echo "$tag_out"
+  fi
+


Thanks a lot 👍

but someone may argue that it's not the purpose of this PR ;-)

myii · 2020-10-30T23:34:21Z

@dafyddj I've taken this PR and rebased on the latest version of this formula. I've then run it with:

$ bin/convert-formula.sh test_the_use-this-template-button

I've grabbed the diff between the resultant first and second commits into a gist for commenting purposes (not necessarily action items, primarily discussion). Working from top-to-bottom (not prioritised):

[L49-L1330] General comment, not to be tackled by this PR: A couple of files deleted with 1,250+ lines removed -- could/should this be done in the first commit rather than the second, to avoid clutter in the git history? Further sections include:
- [L1416-L3287] -- another 1,850+ lines removed.
- [L3465-L3695] -- another 200+ lines removed.
- So a total of 3,300+ lines removed, which could be avoided entirely from the git history of the converted repo?
[L3295-L3297] Wish-list, not for this PR, nor critical in any way: How about the script being clever enough to modify the heading symbols to match the change in line length throughout the whole of README.rst?
[L4332-L4334] Change requested for this PR, as discussed in Slack: Please use # yamllint disable-line rule:line-length instead of the current method; the diff here shows that fix already applied.
[L4391-L4392] Confirming behaviour throughout the Jinja in the file conversions (only the first example linked from the gist): So both of these apply?
- Each hyphen - converted to a single underscore _ -- this is necessary for Jinja variables, IIRC from our conversations.
- Each underscore - converted to a double underscore __ -- this one initially feels a little wonky but probably makes sense overall.

All-in-all, a fantastic bit of work, kudos!

* closes saltstack-formulas#210

dafyddj · 2020-10-31T18:40:01Z

3. [L4332-L4334] Change requested for this PR, as discussed in Slack: Please use # yamllint disable-line rule:line-length instead of the current method; the diff here shows that fix already applied.

Done I'll address the other points another time/elsewhere.

dafyddj · 2020-10-31T18:50:37Z

[L4391-L4392] Confirming behaviour throughout the Jinja in the file conversions (only the first example linked from the gist): So both of these apply?

Each hyphen - converted to a single underscore _ -- this is necessary for Jinja variables, IIRC from our conversations.

Each underscore - converted to a double underscore __ -- this one initially feels a little wonky but probably makes sense overall.

The only manipulation done is to convert a hyphen to a double underscore:
NEW_NAME_PYSAFE=$(echo "$NEW_NAME" | sed 's/-/__/g')
I chose a double underscore (rather than a single) just to retain some information that a change was made.

myii · 2020-10-31T19:09:24Z

Thanks for all of the reviews, guys.

Merged, @dafyddj.

saltstack-formulas-travis · 2020-10-31T19:20:33Z

🎉 This PR is included in version 4.3.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

* saltstack-formulas/template-formula#207

dafyddj requested a review from a team as a code owner September 29, 2020 19:48

vutny approved these changes Sep 30, 2020

View reviewed changes

baby-gnu approved these changes Sep 30, 2020

View reviewed changes

daks approved these changes Sep 30, 2020

View reviewed changes

dafyddj marked this pull request as draft September 30, 2020 14:04

dafyddj force-pushed the fix/convert-dash-underscore branch 3 times, most recently from 2abcc49 to be56eaa Compare October 6, 2020 17:21

baby-gnu approved these changes Oct 7, 2020

View reviewed changes

fix(convert-formula.sh): add -_ to allowed chars in formula name

a999fee

dafyddj force-pushed the fix/convert-dash-underscore branch from be56eaa to facced5 Compare October 28, 2020 23:26

fix(convert-formula.sh): delete all existing tags

7c33601

* closes saltstack-formulas#210

dafyddj force-pushed the fix/convert-dash-underscore branch from facced5 to 7c33601 Compare October 31, 2020 18:38

dafyddj marked this pull request as ready for review October 31, 2020 18:50

dafyddj requested a review from a team as a code owner October 31, 2020 18:50

myii approved these changes Oct 31, 2020

View reviewed changes

myii merged commit 4be96d3 into saltstack-formulas:master Oct 31, 2020

saltstack-formulas-travis added the released label Oct 31, 2020

myii added a commit to myii/ssf-formula that referenced this pull request Dec 17, 2020

feat(template): review PR 207

3f612cf

* saltstack-formulas/template-formula#207

dafyddj deleted the fix/convert-dash-underscore branch November 29, 2024 01:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(convert-formulas.sh): add -_ to allowed chars in formula name #207

fix(convert-formulas.sh): add -_ to allowed chars in formula name #207

dafyddj commented Sep 29, 2020 •

edited

Loading

pull-assistant bot commented Sep 29, 2020

vutny left a comment

baby-gnu commented Sep 30, 2020

noelmcloughlin commented Sep 30, 2020 •

edited

Loading

dafyddj commented Sep 30, 2020 •

edited

Loading

myii commented Sep 30, 2020

myii commented Sep 30, 2020 •

edited

Loading

myii commented Sep 30, 2020 •

edited

Loading

dafyddj commented Sep 30, 2020

dafyddj commented Sep 30, 2020

baby-gnu commented Oct 5, 2020

baby-gnu Oct 7, 2020

baby-gnu Oct 7, 2020

myii commented Oct 30, 2020

dafyddj commented Oct 31, 2020 •

edited

Loading

dafyddj commented Oct 31, 2020

myii commented Oct 31, 2020

saltstack-formulas-travis commented Oct 31, 2020

fix(convert-formulas.sh): add -_ to allowed chars in formula name #207

fix(convert-formulas.sh): add -_ to allowed chars in formula name #207

Conversation

dafyddj commented Sep 29, 2020 • edited Loading

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

pull-assistant bot commented Sep 29, 2020

vutny left a comment

Choose a reason for hiding this comment

baby-gnu commented Sep 30, 2020

noelmcloughlin commented Sep 30, 2020 • edited Loading

dafyddj commented Sep 30, 2020 • edited Loading

myii commented Sep 30, 2020

myii commented Sep 30, 2020 • edited Loading

myii commented Sep 30, 2020 • edited Loading

dafyddj commented Sep 30, 2020

dafyddj commented Sep 30, 2020

baby-gnu commented Oct 5, 2020

baby-gnu Oct 7, 2020

Choose a reason for hiding this comment

baby-gnu Oct 7, 2020

Choose a reason for hiding this comment

myii commented Oct 30, 2020

dafyddj commented Oct 31, 2020 • edited Loading

dafyddj commented Oct 31, 2020

myii commented Oct 31, 2020

saltstack-formulas-travis commented Oct 31, 2020

dafyddj commented Sep 29, 2020 •

edited

Loading

Does this PR introduce a `BREAKING CHANGE`?

noelmcloughlin commented Sep 30, 2020 •

edited

Loading

dafyddj commented Sep 30, 2020 •

edited

Loading

myii commented Sep 30, 2020 •

edited

Loading

myii commented Sep 30, 2020 •

edited

Loading

dafyddj commented Oct 31, 2020 •

edited

Loading