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

Return platform name that is lower case and underscored #228

Merged
merged 2 commits into from
Jan 3, 2018

Conversation

jquick
Copy link
Contributor

@jquick jquick commented Dec 14, 2017

Fixes #191
Fixes inspec/inspec#1732

This change is a remake of #192 with the updated platform work. This change sets platform names to always return downcase and underscored.

Signed-off-by: Jared Quick [email protected]

@jquick jquick requested a review from a team December 14, 2017 17:58
@jquick jquick force-pushed the jq/clean_platform_name branch from 333521c to 8a04b19 Compare December 14, 2017 18:02
@jquick jquick force-pushed the jq/clean_platform_name branch 2 times, most recently from 4a4d766 to 8bfd72d Compare December 15, 2017 13:44
@jquick jquick force-pushed the jq/clean_platform_name branch from 8bfd72d to fb2ee9e Compare December 15, 2017 14:05

def clean_name
name = (@platform[:name] || @name)
# This is a history of name change being used upstream in inspec
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what this comment means.

if name =~ /[A-Z ]/
@name_updated = [name]
name = name.downcase.tr(' ', '_')
@name_updated << name
Copy link
Contributor

Choose a reason for hiding this comment

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

What is @name_updated used for?

@@ -25,7 +26,18 @@ def direct_families
def name
# Override here incase a updated name was set
# during the detect logic
@platform[:name] || @name
@clean_name
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit confusing to me to have an instance_var named @clean_name and a method named clean_name and then use them in different ways.

Can we just default to using the clean_name method, and memoize the cleaned_name in that method so we only do the translation once?

@jquick jquick force-pushed the jq/clean_platform_name branch from d5e1e41 to eb7d1ec Compare December 19, 2017 16:41
@jquick
Copy link
Contributor Author

jquick commented Dec 20, 2017

This can be merged but we will want to wait on cutting a train release until we can aline it with the inspec name change. inspec/inspec#2397

@chris-rock
Copy link
Contributor

This will break Windows checks, right?

@jquick
Copy link
Contributor Author

jquick commented Dec 20, 2017

The inspec PR will keep it from breaking and just warn for now till inspec 2.0

@adamleff
Copy link
Contributor

@chris-rock this is totally a breaking change for Train, but SemVer-wise we're OK since this is still 0.x.

We've got an InSpec change in the pipeline that will gracefully handle it there, giving warnings for users as we initially discussed a month or so ago.

We'll want to cut a Train release with this change, and then cut an InSpec release that pins on that with the deprecation handling @jquick is working on.

Copy link
Contributor

@arlimus arlimus left a comment

Choose a reason for hiding this comment

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

Great work and care to detail, kudos!! @jquick

@arlimus arlimus merged commit 949d549 into master Jan 3, 2018
@arlimus arlimus deleted the jq/clean_platform_name branch January 3, 2018 17:14
@clintoncwolfe clintoncwolfe added Type: Bug Feature not working as expected and removed breaking-change labels Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants