Skip to content

Conversation

@lonegunmanb
Copy link
Contributor

@lonegunmanb lonegunmanb commented Sep 19, 2022

This pull request will enforce lexicographic order for attributes in a locals block. This is the first pull request for issue #22 . We've already implemented these rules in our own plugin, but it'll be a great help if this official plugin can accept these rules. @wata727 will you give this pr a review? Thanks!


// Check checks whether single line comments is used
func (r *TerraformLocalsOrderRule) Check(runner tflint.Runner) error {
files, err := runner.GetFiles()
Copy link
Member

Choose a reason for hiding this comment

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

You can use GetLocals() instead of GetFiles().

func (r *Runner) GetLocals() (map[string]*Local, hcl.Diagnostics) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip, I've checked Local struct, it contains name and Range, which contains file name and range in that file.

What we want to sort is the local argument inside a locals block, but not in a file range, since this Local struct lacks block information, I think it might be easier to analyze them in File->Block->Attribute way, could I keep this way?

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense. In this case, it certainly seems better to analyze by file.

formattedBlock := string(hclwrite.Format([]byte(suggestedBlock)))
return runner.EmitIssue(
r,
fmt.Sprintf("Recommended locals variable order:\n%s", formattedBlock),
Copy link
Member

Choose a reason for hiding this comment

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

It may be a bit difficult to print a suggestion to support both HCL native syntax and JSON syntax. Also, if possible, I would like to avoid multi-line messages. In this case, "local values must be in alphabetical order" would be good.

By the way, it's probably best to provide an autofix for such rules. See terraform-linters/tflint#266

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed multi-line suggestion as you suggested, but when we tried to print a multi-line suggestion, we found it's hard to preserve comment line inside code since all comments have been removed from AST. If we want to sort arguments, we need look-back for comment lines and move them too, so we just skip comments for now. I think if we'd like to implement this autofix feature, we should consider preserving comments lines too.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the hclwrite package has a parser that generates AST nodes containing comments, so we'll need to use it to rewrite the code.

@lonegunmanb
Copy link
Contributor Author

Thanks @wata727 , I'll try to fix as you suggested.

@lonegunmanb lonegunmanb requested a review from wata727 September 22, 2022 07:58
@lonegunmanb
Copy link
Contributor Author

Thanks @wata727 , I've updated this pr, would you please give it another review? Thanks!

@lonegunmanb lonegunmanb requested a review from wata727 September 26, 2022 02:48
@lonegunmanb
Copy link
Contributor Author

Thanks @wata727 , I've corrected code as you suggested, would you please give it another review? Thanks.

Copy link
Member

@wata727 wata727 left a comment

Choose a reason for hiding this comment

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

Great. Thanks!

@wata727 wata727 merged commit af38373 into terraform-linters:main Sep 27, 2022
wata727 added a commit that referenced this pull request Oct 17, 2022
This reverts commit af38373.
@wata727 wata727 mentioned this pull request Oct 17, 2022
wata727 added a commit that referenced this pull request Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants