Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/rules/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ All rules are enabled by default, but by setting `preset = "recommended"`, you c
|[terraform_module_version](terraform_module_version.md)|Checks that Terraform modules sourced from a registry specify a version|✔|
|[terraform_naming_convention](terraform_naming_convention.md)|Enforces naming conventions for resources, data sources, etc||
|[terraform_ordered_locals](terraform_ordered_locals.md)|Recommend proper order for variables in `locals` blocks||
|[terraform_ordered_variables](terraform_ordered_variables.md)|Recommend proper order for variable blocks||
|[terraform_required_providers](terraform_required_providers.md)|Require that all providers have version constraints through required_providers|✔|
|[terraform_required_version](terraform_required_version.md)|Disallow `terraform` declarations without require_version|✔|
|[terraform_standard_module_structure](terraform_standard_module_structure.md)|Ensure that a module complies with the Terraform Standard Module Structure||
Expand Down
80 changes: 80 additions & 0 deletions docs/rules/terraform_ordered_variables.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
# terraform_ordered_variables

Recommend proper order for variable blocks
The variables without default value are placed prior to those with default value set
Then the variables are sorted based on their names (alphabetic order)

## Example

```hcl
variable "docker_ports" {
type = list(object({
internal = number
external = number
protocol = string
}))
default = [
{
internal = 8300
external = 8300
protocol = "tcp"
}
]
}

variable "availability_zone_names" {
type = list(string)
default = ["us-west-1a"]
}

variable "image_id" {
type = string
}
```

```
$ tflint
1 issue(s) found:

Notice: Variables should be sorted in the following order: required(without default value) variables in alphabetical order, optional variables in alphabetical order.

on main.tf line 1:
1: variable "image_id" {

Reference: https://github.com/terraform-linters/terraform/blob/v0.0.1/docs/rules/terraform_ordered_variables.md
```

## Why
It helps to improve the readability of terraform code by sorting variable blocks in the order above.

## How To Fix

Sort variables in the following order: required(without default value) variables in alphabetical order, optional variables in alphabetical order.

For the code in [example](#Example), it should be sorted as the following order:

```hcl
variable "image_id" {
type = string
}

variable "availability_zone_names" {
type = list(string)
default = ["us-west-1a"]
}

variable "docker_ports" {
type = list(object({
internal = number
external = number
protocol = string
}))
default = [
{
internal = 8300
external = 8300
protocol = "tcp"
}
]
}
```
1 change: 1 addition & 0 deletions rules/preset.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ var PresetRules = map[string][]tflint.Rule{
NewTerraformDocumentedVariablesRule(),
NewTerraformEmptyListEqualityRule(),
NewTerraformOrderedLocalsRule(),
NewTerraformOrderedVariablesRule(),
NewTerraformModulePinnedSourceRule(),
NewTerraformModuleVersionRule(),
NewTerraformNamingConventionRule(),
Expand Down
178 changes: 178 additions & 0 deletions rules/terraform_ordered_variable.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
package rules

import (
"github.com/google/go-cmp/cmp"
"sort"

"github.com/hashicorp/hcl/v2"
"github.com/terraform-linters/tflint-plugin-sdk/hclext"
"github.com/terraform-linters/tflint-plugin-sdk/tflint"
"github.com/terraform-linters/tflint-ruleset-terraform/project"
)

// TerraformOrderedVariablesRule checks whether the variables are sorted in expected order
type TerraformOrderedVariablesRule struct {
tflint.DefaultRule
}

// NewTerraformOrderedVariablesRule returns a new rule
func NewTerraformOrderedVariablesRule() *TerraformOrderedVariablesRule {
return &TerraformOrderedVariablesRule{}
}

// Name returns the rule name
func (r *TerraformOrderedVariablesRule) Name() string {
return "terraform_ordered_variables"
}

// Enabled returns whether the rule is enabled by default
func (r *TerraformOrderedVariablesRule) Enabled() bool {
return true
}

// Severity returns the rule severity
func (r *TerraformOrderedVariablesRule) Severity() tflint.Severity {
return tflint.NOTICE
}

// Link returns the rule reference link
func (r *TerraformOrderedVariablesRule) Link() string {
return project.ReferenceLink(r.Name())
}

// Check checks whether the variables are sorted in expected order
func (r *TerraformOrderedVariablesRule) Check(runner tflint.Runner) error {
path, err := runner.GetModulePath()
if err != nil {
return err
}
if !path.IsRoot() {
// This rule does not evaluate child modules.
return nil
}

body, err := runner.GetModuleContent(&hclext.BodySchema{
Blocks: []hclext.BlockSchema{
{
Type: "variable",
LabelNames: []string{"name"},
Body: &hclext.BodySchema{
Attributes: []hclext.AttributeSchema{{Name: "default"}},
},
},
},
}, &tflint.GetModuleContentOption{IncludeNotCreated: true})
if err != nil {
return err
}

variables := r.variablesGroupByFile(body)
for _, blocks := range variables {
if err = r.checkVariableOrder(runner, blocks); err != nil {
return err
}
}
return nil
}

func (r *TerraformOrderedVariablesRule) variablesGroupByFile(body *hclext.BodyContent) map[string]hclext.Blocks {
variables := make(map[string]hclext.Blocks)
for _, b := range body.Blocks {
variables[b.DefRange.Filename] = append(variables[b.DefRange.Filename], b)
}
for _, blocks := range variables {
sort.Slice(blocks, func(i, j int) bool {
return blocks[i].DefRange.Start.Line < blocks[j].DefRange.Start.Line
})
}
return variables
}

func (r *TerraformOrderedVariablesRule) checkVariableOrder(runner tflint.Runner, blocks []*hclext.Block) error {
requiredVars := r.getSortedVariableNames(blocks, false)
optionalVars := r.getSortedVariableNames(blocks, true)
sortedVariableNames := append(requiredVars, optionalVars...)

variableNames := r.getVariableNames(blocks)
if cmp.Equal(variableNames, sortedVariableNames) {
return nil
}

return runner.EmitIssue(
r,
"Variables should be sorted in the following order: required(without default value) variables in alphabetical order, optional variables in alphabetical order.",
r.issueRange(blocks),
)
}

func (r *TerraformOrderedVariablesRule) issueRange(blocks hclext.Blocks) hcl.Range {
requiredVariables := r.getVariables(blocks, false)
optionalVariables := r.getVariables(blocks, true)

if r.overlapped(requiredVariables, optionalVariables) {
return optionalVariables[0].DefRange
}

firstRange := r.firstNonSortedBlockRange(requiredVariables)
if firstRange != nil {
return *firstRange
}
firstRange = r.firstNonSortedBlockRange(optionalVariables)
if firstRange != nil {
return *firstRange
}
panic("expected issue not found")
}

func (r *TerraformOrderedVariablesRule) overlapped(requiredVariables, optionalVariables hclext.Blocks) bool {
if len(requiredVariables) == 0 || len(optionalVariables) == 0 {
return false
}

firstOptional := optionalVariables[0].DefRange
lastRequired := requiredVariables[len(requiredVariables)-1].DefRange

return firstOptional.Start.Line < lastRequired.Start.Line
}

func (r *TerraformOrderedVariablesRule) firstNonSortedBlockRange(blocks hclext.Blocks) *hcl.Range {
for i, b := range blocks {
if i == 0 {
continue
}
previousVariableName := blocks[i-1].Labels[0]
currentVariableName := b.Labels[0]
if currentVariableName < previousVariableName {
return &b.DefRange
}
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

nil causes panic. It should return an empty hcl.Range if there are no range.

Copy link
Contributor Author

@lonegunmanb lonegunmanb Sep 28, 2022

Choose a reason for hiding this comment

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

Hi, I left this nil by intention, please allow me to explain. this issueRange is called when we're sure that there is at least one violation inside these blocks, otherwise the caller method checkVariableOrder should return a nil as error instead. Let's say somehow we've a bug in this issueRange method that causes we cannot find any violation, which is wrong, an empty hcl.Range will confuse our user, make them wonder what this line 0 issue stands for. In this scenario I think maybe it's better to let it crash so we can receive a bug report that this method somehow has a bug. I'll leave this decision to you, if you prefer an empty hcl.Range, I'll be happy to change it in your way.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. In that case, I think calling panic explicitly is correct.
It is better to change the return type to hcl.Range instead of *hcl.Range, since it is not necessary to return nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Would you please give another review? Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the confusion. I would like to discuss about #22 (comment) before restarting the review.

}

func (r *TerraformOrderedVariablesRule) getVariableNames(blocks hclext.Blocks) []string {
var variableNames []string
for _, b := range blocks {
variableNames = append(variableNames, b.Labels[0])
}
return variableNames
}

func (r *TerraformOrderedVariablesRule) getSortedVariableNames(blocks hclext.Blocks, defaultWanted bool) []string {
var names []string
filteredBlocks := r.getVariables(blocks, defaultWanted)
for _, b := range filteredBlocks {
names = append(names, b.Labels[0])
}
sort.Strings(names)
return names
}

func (r *TerraformOrderedVariablesRule) getVariables(blocks hclext.Blocks, defaultWanted bool) hclext.Blocks {
var c hclext.Blocks
for _, b := range blocks {
if _, hasDefault := b.Body.Attributes["default"]; hasDefault == defaultWanted {
c = append(c, b)
}
}
return c
}
Loading