Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Roles rules #19

Closed
jaymzh opened this Issue · 16 comments

2 participants

@jaymzh
Collaborator

How do you feel about implementing roles rules?

I'm about to write an SVN pre-commit that enforces the name of the role file is also the name of the role, which is easy to do outside of FC, but since you can define attributes and stuff in roles, it may be cool to have the FC framework for them.

@acrmp
Owner

Hi Phil,

Sounds useful. To flesh this out a bit more:

  • Add a new option -R to specify the role path. Accepts a directory or file path.
  • Add role to the rule dsl.
  • Do you want to be able to call foodcritic with a role path but without a cookbook path?

Rules that could be added using this:

  • Has a run_list been defined (either default or in env_run_lists)?
  • Can the roles and recipes in the run list be resolved?

For now I'd leave out:

  • Support for roles expressed as json
  • Inferring the role path from the cookbook path

Thanks,

Andrew.

@jaymzh
Collaborator

Yeah. I'd need to be able to call it without a cookbook - we run FC as a svn pre-commit, so if no cookbook was changed in the precommit, there's nothing to check. Which begs the question, perhaps FC should take -C and -R, and, optionally, at some point in the future perhaps drop the "arguments are cookbook paths" behavior?

Both of those checks sound awesome to me.

I agree there's no reason to support JSON roles since those are deprecated anyway... and yeah, we shouldn't infer paths.

@acrmp
Owner

-C is already reserved for --context at present. So is -r for REPL so it gets messy quickly.

We could take a type flag and keep the same foodcritic [path] interface.

These would be equivalent:

  • foodcritic -T role my_role_path
  • foodcritic --lint-type role my_role_path

These would be equivalent:

  • foodcritic my_cookbook_path
  • foodcritic -T cookbook my_cookbook_path
  • foodcritic --lint-type cookbook my_cookbook_path

This would mean that you couldn't check both cookbooks and roles in one command.

@jaymzh
Collaborator

how about -c for cookbooks and -R for roles? or -B for cookBooks?

I don't like the type option... that makes me sad.

@acrmp
Owner

Can you elaborate on why?

I prefer type to mixed case which would annoy me because I'd expect the option case to be consistent and guess incorrectly. Type means we could also extend linting to environments or other stuff without an exploding list of cmd options.

@jaymzh
Collaborator

Because (1) My primary use-case is a precomit where I want to be able to lint everything in one pass and (2) I don't want to have to run FC twice for myself or make other people do it either.

If you want consistent cases then -R for roles and -B for cookbooks?

@acrmp
Owner

I think you're right that the ability to lint everything in one go trumps misgivings about the number of arguments.

So just to summarise for clarity:

These would be equivalent:

  • foodcritic my_cookbook_path
  • foodcritic -B my_cookbook_path
  • foodcritic --cookbook-path my_cookbook_path

These would be equivalent:

  • foodcritic -R my_role_path
  • foodcritic --role-path my_role_path

Leaving us open for:

  • foodcritic -E my_environment_path
  • foodcritic --environment-path my_environment_path

Ok?

@acrmp
Owner

And these would all be valid:

foodcritic -B my_cookbook_path -R my_role_path
foodcritic -R my_role_path my_cookbook_path
foodcritic -R my_role_path -B my_cookbook_path -B my_other_cookbook_path
foodcritic -B my_cookbook_path
@jaymzh
Collaborator

Perfect!

@acrmp
Owner

Awesome. I'll look to implement this soon.

Thank you for contributing your ideas and making foodcritic better - keep 'em coming!

@jaymzh
Collaborator

Is the filename already available to rules? If not, I think it'll be particularly useful for roles and environments...

@acrmp
Owner

The filename is passed as the second argument to the block:

recipe do |ast,filename|
  # ...
end

In the case of the cookbook block there is only a single argument (the cookbook directory).

@acrmp
Owner

Sorry for the delay - I haven't forgotten about this.

@jaymzh
Collaborator

Any updates?

@jaymzh
Collaborator

Ping?

@acrmp acrmp referenced this issue from a commit
@acrmp Add roles support with -R flag, refs #19.
- FC049: Role name does not match containing file name
- FC050: Name includes invalid characters

Currently we only understand roles in Ruby format. Roles must be in a
role path (specified with --role-path or -R) to be checked. Cookbook
paths may now alternatively be specified with --cookbook-path / -B.

This commit introduces breaking changes to the FoodCritic::Linter
interface.
11fca11
@acrmp
Owner

Hi Phil,

This was released in foodcritic 3.0.0.

Thanks,

Andrew.

@acrmp acrmp closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.