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

Added platform-name wildcard support #5302

Merged
merged 6 commits into from
Dec 3, 2020

Conversation

yarick
Copy link
Contributor

@yarick yarick commented Nov 8, 2020

Added platform-name wildcard support
Added unit test for platform-name wildcard
Added doc for release wildcard
Added doc for platform-name wildcard

Signed-off-by: superyarick yarick@yarick.net

Description

Added a function to convert wildcartds in platform-name to match against regex - borrowed from existing code for matching 'release's

Note

The current case statement that evaluates the supports statements does not allow for usage of both platform, release and platform-name in conjunction as it is a fast failing case statement. I will open a follow-on issue with respect to this.
https://github.com/inspec/inspec/blob/master/lib/inspec/resources/platform.rb#L76-L88

Related Issue

Did not open an issue. Just submitted small fix.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New content (non-breaking change)
  • Breaking change (a content change which would break existing functionality or processes)

Checklist:

  • I have read the CONTRIBUTING document.

added unit test for platform-name wildcard
added doc for release wildcard
added doc for platform-name wildcard

Signed-off-by: superyarick <yarick@yarick.net>
@yarick yarick requested a review from a team as a code owner November 8, 2020 20:40
@netlify
Copy link

netlify bot commented Nov 8, 2020

Deploy preview for chef-inspec ready!

Built with commit e83c0b3

https://deploy-preview-5302--chef-inspec.netlify.app

@chef-expeditor
Copy link
Contributor

chef-expeditor bot commented Nov 8, 2020

Hello yarick! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. Possible Outcomes
    a. If everything looks good, one of them will approve it, and your PR will be merged.
    b. The maintainer may request follow-on work (e.g. code fix, linting, etc). We would encourage you to address this work in 2-3 business days to keep the conversation going and to get your contribution in sooner.
    c. Cases exist where a PR is neither aligned to Chef InSpec's product roadmap, or something the team can own or maintain long-term. In these cases, the maintainer will provide justification and close out the PR.

Thank you for contributing!

@aaronlippold
Copy link
Collaborator

Given this was a clean fork of inspec as of today - now sure why we are getting the ruby-2.4 issues

@Schwad
Copy link
Contributor

Schwad commented Nov 9, 2020

@aaronlippold I'm wondering if there was a bump of the parallel gem which killed 2.4. I'll raise some issues around this

Gem::RuntimeRequirementNotMetError: parallel requires Ruby version >= 2.5

This was referenced Nov 9, 2020
@aaronlippold
Copy link
Collaborator

@Schwad so can we still move ahead with peer review on the code and docs while that parallel issue is being resolved with the backend team?

@Schwad
Copy link
Contributor

Schwad commented Nov 10, 2020

@aaronlippold

so can we still move ahead with peer review on the code and docs while that parallel issue is being resolved with the backend team?

Yes, peer review may continue whilst this is being resolved.

@Schwad
Copy link
Contributor

Schwad commented Nov 11, 2020

@yarick @aaronlippold we have merged the parallel fix so you can rebase and this should pass now 💯

@aaronlippold
Copy link
Collaborator

aaronlippold commented Nov 11, 2020 via email

@aaronlippold
Copy link
Collaborator

new updates from maser merged in.

@aaronlippold
Copy link
Collaborator

aaronlippold commented Nov 12, 2020

Although this works - we do have a small bit of unexpected or at least unclear behavior - which we documented as a side step - but the situation I would like to clean up as a follow on would be.

The case statement could be made to support more logical filtering in its organization. As you can see from the note, if we do something like:

supports:
  - platform-family: windows
  - platform-name: windows-server-2016*

we pass fast on the case statement just be happy it got windows back and support 2019 or 2012 etc

As documented in this PR - if you want to filter on a specific sub-family by name you need to just use:

supports:
  - platform-name: windows-server-2016* 

I think however the real user community would expect a behavior more along the lines of:

supports:
  - platform-family: windows
    name: *2016*

or, even a bit more detail, given that both 2016 and 2019 have a family of releases in the 10.0.1* range

supports:
  - platform-family: windows
    name: *2019*
    release: 10.0.1.17*

@aaronlippold
Copy link
Collaborator

The use case here is that I want a wrapper profile that I run via a lambda or ssm function that just has all my os baselines for windows and or linux and just runs the right one for the right platform ...

@aaronlippold
Copy link
Collaborator

@Schwad what's up with buildkit?

@aaronlippold
Copy link
Collaborator

Hi all, are we good to go on merging this? Thanks.

@mjingle
Copy link
Contributor

mjingle commented Nov 30, 2020

@yarick Hello! Thank you for updating the documentation with your code. Please incorporate my suggested documentation edits, and then you will good to go from a docs perspective. Thank you in advance and let me know if you have any questions.

@yarick
Copy link
Contributor Author

yarick commented Dec 1, 2020

@mjingle - I believe I have addressed all your comments. Let me know if there is anything else.

@mjingle mjingle self-requested a review December 2, 2020 15:16
Copy link
Contributor

@mjingle mjingle left a comment

Choose a reason for hiding this comment

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

@yarick Your docs update looks great! Thank you so much. You are good to go from a docs perspective. 👍

Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Thanks so much!

@Schwad
Copy link
Contributor

Schwad commented Dec 3, 2020

@aaronlippold @yarick thanks much for this! Merging now.

In future please squash your PRs into one commit (unless there's a good reason from a documentation standpoint not to)

@Schwad Schwad merged commit c766519 into inspec:master Dec 3, 2020
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.

None yet

5 participants