Issue warning during check if profile name contains slash #2231
Conversation
lib/inspec/metadata.rb
Outdated
| @@ -112,6 +115,10 @@ def valid # rubocop:disable Metrics/AbcSize | |||
| errors.push("Missing profile #{field} in #{ref}") | |||
| end | |||
|
|
|||
| if params[:name] =~ %r{[\/\\]} | |||
| warnings.push("Profile names containing slashes (#{params[:name]}) are deprecated.") | |||
There was a problem hiding this comment.
Can I suggest changing this to be a bit more explicit? Perhaps:
Your profile name (#{params[:name}) contains a slash which will not be permitted in InSpec 2.0. Please change your profile name in the `inspec.yml` file.
lib/inspec/metadata.rb
Outdated
| metadata.params[:name] = "tests from #{original_target}" unless original_target.to_s.empty? | ||
| # Crudely slug the target to not contain slashes, to avoid breaking | ||
| # unit tests that look for warning sequences | ||
| metadata.params[:name] = "tests from #{original_target}".gsub(%r{[\\\/]}, '.') unless original_target.to_s.empty? |
There was a problem hiding this comment.
What do you think about using hyphens instead of periods? That's a more common word separator when slugging a URL, based on absolutely zero evidence I have to provide to you except my gut instinct and some googling. :)
I also wonder if this change is necessary... should we put this line in the title metadata instead and put something else in the name?
There was a problem hiding this comment.
I was debating between dashes, underscores, and "something else shell-safe" (I settled on a period, as you see). My concern with either dashes or underscores is that they're both common in both directory names and profile names, so slugging would lose the path separator....
my/profiles/from-steve/no-shenanigans/inspec.yml =>
my-profiles-from-steve-no-shenanigans
Maybe that's fine.
Placing it (unslugged, as originally generated) in title works fine. What should we put in name ? "default" ?
There was a problem hiding this comment.
I'm pretty agnostic to the hyphens vs periods. I'll let someone else weigh in on that.
I guess what I'm coming back to on this particular change is if this is even necessary since this line is only hit when we're dealing with a file of tests outside of the construct of a profile. I suspect it's not even needed, but given your comment about wishing to potentially catch this in unit tests, I'm totally fine as-is.
There was a problem hiding this comment.
Since the CLI output will look funky when we strip the slashes, and profiles need to have names, how about we put the actual path as-is in the title so that way it at least shows in the output unaltered?
clintoncwolfe
force-pushed
the
cw/warn-on-slashes-on-check
branch
from
94fd0e5 to
966cb71
Compare
…s in their name Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
…it tests Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
clintoncwolfe
force-pushed
the
cw/warn-on-slashes-on-check
branch
from
f133832 to
a827407
Compare
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
| @@ -108,7 +108,6 @@ | |||
| it 'prints loads of warnings' do | |||
| logger.expect :info, nil, ["Checking profile in #{home}/mock/profiles/#{profile_id}"] | |||
| logger.expect :error, nil, ["Missing profile version in inspec.yml"] | |||
| logger.expect :warn, nil, ["Missing profile title in inspec.yml"] | |||
There was a problem hiding this comment.
I would expect that we see the result, if the inspec.yml is empty. Am I missing something?
There was a problem hiding this comment.
If the inspec.yml is missing, we generate a profile name. Previously that name contained the path to the profile, which likely contained slashes. As that's not permitted, we replaced the slashes; but to retain clarity, we now store the original path in the title.
So, a profile without metadata will now have both a name and a title generated, and thus there will be no warning about a missing title.
Discussion here: #2231 (comment)
There was a problem hiding this comment.
@chris-rock this makes sense to me. It's only in the case where a profile exists, an inspec.yml exists, and the file is empty (or doesn't include a name). By default, we'll create a default name that includes the path (slugged), and a title that contains the actual path.
Since in the case of empty metadata / no name, we'll always have a title, then it's right to remove this test assertion.
There was a problem hiding this comment.
Thank you @clintoncwolfe for this improvement and the great discussion!
Fixes #1954 . Also, in the case that no name can be determined and a name must be generated from the target (which is common in unit testing), replace slashes with dots. This is sensible if the default name is arbitrary; if real-world use relies on having profiles with names like "tests from /path/to/tests", we can update the unit tests instead; however any real-world processes would have issues in InSpec 2.0 anyway due to #1783 .