saltstack-formulas / users-formula

Configure users via pillar
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
99 stars 362 forks source link

Add test vimrc generation #214

Closed fzipi closed 4 years ago

fzipi commented 4 years ago

This PR adds tests to verify that .vimrc is being correctly generated.

Related to #213 .

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?

No.

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

myii commented 4 years ago

@fzipi If you permit me, I'll "hijack" this PR to merge in all of the changes we discussed in #213. That will include some modifications to what you've provided here, to match the general structure we are using across all of our formulas. I'll ensure the attribution for these parts will remain with you. Is that OK with you?

myii commented 4 years ago

One thing I'm thinking about is limiting the users.vimrc state to only run for one of the instances. I've just run this across all of the platforms (https://travis-ci.org/myii/users-formula/builds/605195734) and it adds 20-40 seconds per instance.

fzipi commented 4 years ago

For sure!

myii commented 4 years ago

@fzipi OK, here's what I'm proposing to push here to replace the current commits (due to amendments):

This passes in Travis:

Specifically the new vimrc suite (which runs the default suite tests as well):

I'll await your feedback before going forward.

fzipi commented 4 years ago

This adds lots of quality on top. It is perfect! Thanks again!

myii commented 4 years ago

@fzipi Thanks for the review. I'm usually able to push additional commits to a contributor's branch but I can't force push to it, to replace the existing commits. So which of the following appeals to you?

  1. You could temporarily invite me as a collaborator to your fork of this formula, then I could force push and we could get this merged using this PR only.
  2. I could add only my commits to another PR, get that merged in first and then you could adjust this PR to contain your modified commits, to be merged afterwards.
  3. I could add all of the commits I linked to above (myii/users-formula@a1ef7e57d962^...86144be) in a separate PR and then we could close this PR.
fzipi commented 4 years ago

Sorry @myii , realised I owed you the answer! My apologies.

I would say let's close this one, and and let's create a new one with yours. So 3) :)

myii commented 4 years ago

Thanks @fzipi, I've created #216 with your (amended) commits included.