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

Testing cleanup #48

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

Conversation

dafyddj
Copy link
Contributor

@dafyddj dafyddj commented Oct 7, 2019

Accomplish two main things:

  • avoid upstream inspec bugs in describe.one by cleaning up inspec profiles
  • add an automated test for binary package upgrades - important as this is something
    that was broken in the past (and was almost broken again in a subsequent refactor)

  * avoiding use of `describe.one` due to upstream bug inspec/4547
    results in cleaner code
  * avoid repetition of tests between dev and prod server profiles
  * as problematic controls removed in previous commit
@dafyddj dafyddj requested a review from myii October 7, 2019 22:32
@myii
Copy link
Member

myii commented Oct 8, 2019

@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! saltstack-formulas/template-formula#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
Copy link
Contributor Author

dafyddj commented Oct 8, 2019

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
Copy link
Member

myii commented Oct 8, 2019

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

@aboe76
Copy link
Member

aboe76 commented Oct 8, 2019

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

@myii
Copy link
Member

myii commented Oct 8, 2019

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

@aboe76
Copy link
Member

aboe76 commented Oct 8, 2019

Let me know when you want them merged.

@myii
Copy link
Member

myii commented Oct 8, 2019

@aboe76 I'm almost done with saltstack-formulas/template-formula#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.

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.

3 participants