Add rules to enforce snake_case names #697

jgeurts · 2020-04-02T12:18:34Z

Adds the following rules:

terraform_snake_case_data_source_name
terraform_snake_case_local_name
terraform_snake_case_module_name
terraform_snake_case_output_name
terraform_snake_case_resource_name
terraform_snake_case_variable_name

Please let me know if anything needs changing or if these rules would be more appropriate elsewhere.

wata727 · 2020-04-04T09:35:32Z

Hi @jgeurts! Thank you for your contribution.

It's so great, but these rules look very similar to existing terraform_dash_in_* rules.
https://github.com/terraform-linters/tflint/blob/master/docs/rules/terraform_dash_in_data_source_name.md

I think it is better to make these rules into one rule with configurable style options. For instance, terraform_module_pinned_source rule has a style option to select enforced style.
https://github.com/terraform-linters/tflint/blob/master/docs/rules/terraform_module_pinned_source.md

What do you think?

jgeurts · 2020-04-04T14:58:45Z

Sounds good! I'm curious to hear your thoughts on the following name/options:

Name rule terraform_naming_convention with the following options:

Name	Default	Value
enabled	true	Boolean
options	{ selector = "default", format = "snake_case" }	Array

Options

Array of configuration options:

Name	Default	Value
selector	default	default, data, local, module, output, resource, variable
format	snake_case	snake_case, UPPER_CASE, camelCase, strictCamelCase, PascalCase, StrictPascalCase, kebab-case, null
custom	null	{ regex = String, match = Boolean }, null

`format`

The format option defines the allowed formats for the identifier. This option accepts an array of the following values, and the identifier can match any of them:

camelCase - standard camelCase format - no underscores are allowed between characters, and consecutive capitals are allowed (i.e. both myID and myId are valid).
strictCamelCase - same as camelCase, but consecutive capitals are not allowed (i.e. myId is valid, but myID is not).
PascalCase - same as camelCase, except the first character must be upper-case.
StrictPascalCase - same as strictCamelCase, except the first character must be upper-case.
snake_case - standard snake_case format - all characters must be lower-case, and underscores are allowed.
UPPER_CASE - same as snake_case, except all characters must be upper-case.
kebab-case - all characters must be lower-case, and dashes are allowed.
null - signifies "this selector shall not have its format checked".
This can be useful if you want to enforce no particular format for a specific selector, after applying a group selector.

`custom`

The custom option defines a custom regex that the identifier must (or must not) match. This option allows you to have a bit more finer-grained control over identifiers, letting you ban (or force) certain patterns and substrings.
Accepts an object with the following properties:

regex - String representing a golang regular expression
match - true if the identifier must match the regex, false if the identifier must not match the regex.

Examples:

Default - enforce snake_case for everything

rule "terraform_naming_convention" {
  enabled = true
}

Override for specific block type

rule "terraform_naming_convention" {
  enabled = true
  options = [
    {
      selector = "default"
      format = "snake_case"
    },
    {
      selector = "module"
      format = "kebab-case"
    }
  ]
}

Disable for specific block type

rule "terraform_naming_convention" {
  enabled = true
  options = [
    {
      selector = "default"
      format = "snake_case"
    },
    {
      selector = "output"
      format = null
    }
  ]
}

Custom format

rule "terraform_naming_convention" {
  enabled = true
  options = [
    {
      selector = "default"
      custom = {
        regex = "^[a-zA-Z_-]{2,}$"
        match = true
      }
    }
  ]
}

wata727 · 2020-04-05T07:39:12Z

It sounds good, but I have some suggestions.

First, I'm thinking whether this rule should be disabled by default. Styling issues often do not affect the actual Terraform workflow. I believe it is best to minimize the issues that appear by default.

Second, the configuration language would be smarter in the following format:

rule "terraform_naming_convetion" {
  enabled = true

  default {
    format = "snake_case"
  }

  resource {
    custom {
      regex = "^[a-zA-Z_-]{2,}$"
      match = true
    }
  }
}

Finally, I have two small concerns:

There is no alternative option for terraform_dash_in_* rules. I think snake_case is an alternative in many cases, but I'm not sure that I can follow all users...
I'm not sure if we need to support this many format options. For example, kebab-case clearly violates best practice.

jgeurts · 2020-04-05T12:34:19Z

Ah nice, I like that config language alternative! Think we would need the default wrapper? For example:

rule "terraform_naming_convention" {
  enabled = true

  format = "snake_case"

  resource {
    custom {
      regex = "^[a-zA-Z_-]{2,}$"
      match = true
    }
  }
}

I’m not sure I understand the first bullet point for an alternative option. Could you explain that a bit more?

I’m good to remove options from format. It could go with only snake_case to begin with unless you’d like any others.

wata727 · 2020-04-05T15:14:00Z

Think we would need the default wrapper?

Good suggestion. Your example is cooler than mine 👍

I’m not sure I understand the first bullet point for an alternative option. Could you explain that a bit more?

For example, the terraform_dash_in_* rules simply check that dash is not included in the name, so names like example_FooBar are also allowed. When removing the terraform_dash_in_* rules and integrating them into the terraform_naming_convention rule, it is not possible to enforce names that do not contain dashes while allowing such names. We will need to provide a format like no_dash to support this.

I’m good to remove options from format. It could go with only snake_case to begin with unless you’d like any others.

This might be good. Unless someone needs another format, minimal implementation would be good.

jgeurts · 2020-04-06T14:35:22Z

Sounds good, I'll work on those changes and will comment back here when I have something to review!

jgeurts · 2020-04-10T14:07:14Z

@wata727 I could use some help, if/when you have some time. This is my first time working with go, so I welcome a strong critique of the code. I'm not familiar with many golang conventions or approaches to keep things concise.

The main part I'm struggling with is how to get the tests to recognize my rule configuration. I have a handful of tests stubbed out for data block naming conventions, but I'm getting errors like Unsupported argument; An argument named "format" is not expected here. or Unsupported argument; An argument named "custom" is not expected here.. Both of which relate to the syntax for the rule.

Any advice would be appreciated!

wata727 · 2020-04-11T13:20:34Z

rules/terraformrules/terraform_naming_convention.go

+type TerraformNamingConventionRule struct{}
+
+type terraformNamingConventionRuleConfig struct {
+	BlockFormatConfig


format and custom tags are not recognized when embedding a struct. It needs to be modified as follows:

Suggested change

BlockFormatConfig

Format string `hcl:"format,optional"`

Custom string `hcl:"custom,optional"`

By the way, embedding of Data etc. works correctly because the hcl:"data,block" tag is recognized to the struct itself.

jgeurts · 2020-04-12T23:02:00Z

@wata727 Ok, this should be ready for review. Thank you for your help with the changes to this and with my golang programming!

wata727

Nice! I have left some comments so please take a look :)

wata727 · 2020-04-18T12:30:19Z

docs/rules/terraform_naming_convention.md

@@ -0,0 +1,238 @@
+# terraform_snake_case_data_source_name


The title seems to remain old.

wata727 · 2020-04-18T12:37:27Z

docs/rules/terraform_naming_convention.md

+
+## How To Fix
+
+Use lower-case characters and separate words with underscores (`_`)


This also seems to be old. The way to fix will depend on the rule configuration.

wata727 · 2020-04-18T12:41:24Z

docs/rules/terraform_naming_convention.md

+
+* `snake_case` - standard snake_case format - all characters must be lower-case, and underscores are allowed.
+* `mixed_snake_case` - modified snake_case format - characters may be upper or lower case, and underscores are allowed.
+* `""` - empty string - signifies "this selector shall not have its format checked".


[IMO] I guess that making an empty string meaningful is a bit counterintuitive. It would be nice to assign a dedicated word such as "none", what do you think?

wata727 · 2020-04-18T12:46:03Z

docs/rules/terraform_naming_convention.md

+
+Name | Default | Value
+--- | --- | ---
+enabled | `true` | Boolean


wata727 · 2020-04-18T12:51:56Z

rules/terraformrules/terraform_naming_convention.go

+
+// Severity returns the rule severity
+func (r *TerraformNamingConventionRule) Severity() string {
+	return tflint.WARNING


[IMO] I think that the "notice" is appropriate for violating the naming convention. What do you think?

wata727 · 2020-04-18T13:04:31Z

rules/terraformrules/terraform_naming_convention.go

+		nameValidator := &NameValidator{
+			IsNamedFormat: false,
+			Format:        custom,
+			Regexp:        regexp.MustCompile(custom),


MustCompile panics if the custom pattern cannot be parsed.
https://golang.org/pkg/regexp/#MustCompile

wata727 · 2020-04-18T13:16:06Z

rules/terraformrules/terraform_naming_convention.go

+			nameValidator := &NameValidator{
+				IsNamedFormat: true,
+				Format:        format,
+				Regexp:        regexp.MustCompile("^[a-z][a-z0-9]*(_[a-z0-9]+)*$"),


You can reduce the cost of compiling each time by compiling these pre-defined regexp as package global variables. In this case, it would be good to use MustCompile.

mveitas · 2020-04-25T14:08:12Z

The rule needs to be added to the providers.go as well: https://github.com/terraform-linters/tflint/blob/master/rules/provider.go#L23

# Conflicts: # docs/rules/README.md

jgeurts · 2020-04-25T16:17:22Z

@wata727 and @mveitas The latest commits should address all of the review comments. Please let me know if I missed anything or if I can make any other changes to this!

mveitas · 2020-04-26T14:09:12Z

rules/terraformrules/terraform_naming_convention.go

+	}
+
+	// Actually run any checks for modules
+	if dataNameValidator != nil {


I might make sense to move the logic if handling each of the block types into their own methods

Thanks for the suggestion, I moved the code to separate methods. I'd like to make things a bit more concise, as there's a fair amount of duplication, but I don't think my go abilities & available time allow for that.

jgeurts · 2020-04-27T14:48:23Z

Not sure how to kick off the build action again.

GitHub Actions has encountered an internal error when running your job.

wata727 · 2020-04-29T13:24:06Z

Not sure how to kick off the build action again.

Rerunned.

wata727

Awesome! Thank you for a great job!

jgeurts added 2 commits April 2, 2020 06:47

Add rules to enforce snake_case

232cc22

Add documentation for snake_case rules

9a74f48

jgeurts added 3 commits April 8, 2020 18:37

Replace terraform_snake_case rules with terraform_naming_convention

85dbbee

Fix some rule and code style issues

fd527e1

Add tests for data block configurations

660885f

wata727 reviewed Apr 11, 2020

View reviewed changes

jgeurts added 7 commits April 12, 2020 10:34

Fix default format and custom properties

0790e0c

Add tests for local values

b1e225e

Add module tests

f7dcc9c

Add tests for output blocks

c0f7091

Add tests for resource blocks

b73a824

Add tests for variable blocks

8c16828

Add documentation for terraform_naming_convention

e7d2192

wata727 reviewed Apr 18, 2020

View reviewed changes

jgeurts added 7 commits April 25, 2020 10:42

Merge remote-tracking branch 'upstream/master' into snake_case_rules

206b111

# Conflicts: # docs/rules/README.md

Add naming convention rule

ca03eb2

Fix title

b6f3b8e

Fix default value for enabled

fac3d80

Fix description for how to fix

671d241

Change severity to NOTICE

19207de

Use “none” instead of “” to disable format

5c57b53

jgeurts added 3 commits April 25, 2020 11:11

Store known format regex as global variables

3ac452f

Return error instead of panic if custom regex does not compile

4e48ab0

Fix formatting

5ab67e1

mveitas reviewed Apr 26, 2020

View reviewed changes

Move block validation to separate methods

76be51f

wata727 approved these changes Apr 29, 2020

View reviewed changes

wata727 merged commit 7e6b34b into terraform-linters:master Apr 29, 2020

jgeurts deleted the snake_case_rules branch April 29, 2020 14:20

wata727 mentioned this pull request May 4, 2020

Remove terraform_dash_in_* rules #737

Merged

wata727 mentioned this pull request Jul 30, 2020

feat: support named custom formats in terraform_naming_convention rule #857

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rules to enforce snake_case names #697

Add rules to enforce snake_case names #697

jgeurts commented Apr 2, 2020

wata727 commented Apr 4, 2020

jgeurts commented Apr 4, 2020 •

edited

wata727 commented Apr 5, 2020

jgeurts commented Apr 5, 2020

wata727 commented Apr 5, 2020

jgeurts commented Apr 6, 2020

jgeurts commented Apr 10, 2020

wata727 Apr 11, 2020

jgeurts commented Apr 12, 2020 •

edited

wata727 left a comment

wata727 Apr 18, 2020

wata727 Apr 18, 2020

wata727 Apr 18, 2020

wata727 Apr 18, 2020

wata727 Apr 18, 2020

wata727 Apr 18, 2020

wata727 Apr 18, 2020

mveitas commented Apr 25, 2020

jgeurts commented Apr 25, 2020

mveitas Apr 26, 2020

jgeurts Apr 27, 2020

jgeurts commented Apr 27, 2020

wata727 commented Apr 29, 2020

wata727 left a comment

	BlockFormatConfig
	Format string `hcl:"format,optional"`
	Custom string `hcl:"custom,optional"`


		## How To Fix

		Use lower-case characters and separate words with underscores (`_`)

Add rules to enforce snake_case names #697

Add rules to enforce snake_case names #697

Conversation

jgeurts commented Apr 2, 2020

wata727 commented Apr 4, 2020

jgeurts commented Apr 4, 2020 • edited

Options

format

custom

Examples:

Default - enforce snake_case for everything

Override for specific block type

Disable for specific block type

Custom format

wata727 commented Apr 5, 2020

jgeurts commented Apr 5, 2020

wata727 commented Apr 5, 2020

jgeurts commented Apr 6, 2020

jgeurts commented Apr 10, 2020

Choose a reason for hiding this comment

jgeurts commented Apr 12, 2020 • edited

wata727 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mveitas commented Apr 25, 2020

jgeurts commented Apr 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgeurts commented Apr 27, 2020

wata727 commented Apr 29, 2020

wata727 left a comment

Choose a reason for hiding this comment

jgeurts commented Apr 4, 2020 •

edited

`format`

`custom`

jgeurts commented Apr 12, 2020 •

edited