search

saltstack-formulas / vault-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
15 stars 59 forks source link

Testing cleanup #48

Open dafyddj opened 4 years ago

dafyddj commented 4 years ago

Accomplish two main things:

myii commented 4 years ago

@dafyddj Good job, I like the look of this (scanned in general, not in detail yet). However, it's come at a very "interesting" time, which you're also responsible for! https://github.com/saltstack-formulas/template-formula/pull/175 is a whole new take on how improve things on Travis, including adding salt-lint and rubocop. Once merged there, the plan is to propagate that to all of the other semantic-release formulas, including this one, using the ssf-formula. Unpicking the merge between this and that is going to take a some co-operation.

These are the options that are apparent to me right now:

  1. We merge in the automated changes and then get this PR rebased on top.
  2. We get this merged in first, in a way that keeps things relatively simple for the automated changes.
  3. Worst case, I use the TOFS-based overrides in ssf-formula to manage the vault-formula files separately from the rest of the formulas.

I don't believe it's fair for me to request number 1. Number 3 is a really ugly workaround; the whole point of the TOFS overrides is almost like a to-do list for things to be standardised across the whole org (and then remove the TOFS override -- such as the current Gemfile for this formula). For number 2, I may need to ask you to leave some of the non-critical changes until a future PR, such as some of the modifications made to inspec.yml. I'd have to review this PR in more detail to figure that out.

What are your thoughts at this stage? Do you have a different opinion about how to proceed, or can you see an alternative option?

dafyddj commented 4 years ago

Yes, I noticed your new PR some time after creating this, and realised they would conflict a little bit. I appreciate that you don't want to make this request, but I think it's probably easier to go for option 1 and merge in the automated changes and updated linting etc., and I'll rebase after that is done.

myii commented 4 years ago

@dafyddj That's very kind of you to offer, I appreciate it.

aboe76 commented 4 years ago

@myii and @dafyddj If you need help please let me know.

myii commented 4 years ago

Thanks @aboe76. We're going to merge https://github.com/saltstack-formulas/template-formula/pull/175 soon, spread those changes to all of the formulas including this one and then revisit this PR again, rebased.

aboe76 commented 4 years ago

Let me know when you want them merged.

myii commented 4 years ago

@aboe76 I'm almost done with https://github.com/saltstack-formulas/template-formula/pull/175. I'm going to run tests on all formulas in my own forks in Travis, to make sure the propagation is going to work as expected. Please have a look at it in the meantime, if you get a chance. Just in case there's something we've missed.