Skip to content

Add lint for db_instance's engine attribute #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Feb 11, 2021
Merged

Add lint for db_instance's engine attribute #61

merged 6 commits into from
Feb 11, 2021

Conversation

chaspy
Copy link
Contributor

@chaspy chaspy commented Feb 7, 2021

ref: #60

@chaspy chaspy force-pushed the chaspy/lint-rds-engine branch from 5ec6e63 to 2c3528c Compare February 7, 2021 18:04
@chaspy chaspy marked this pull request as ready for review February 7, 2021 18:11
@chaspy
Copy link
Contributor Author

chaspy commented Feb 7, 2021

@wata727
Could you review this PR?
I'm afraid I don't still understand how the ruleset is used though, I wrote this with reference to aws_db_instance_invalid_type.go

@chaspy chaspy changed the title Add lint for db_instance engine Add lint for db_instance's engine attribute Feb 7, 2021
wata727
wata727 reviewed Feb 8, 2021
View reviewed changes
Copy link
Member

@wata727 wata727 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I left a suggestion but looks good overall 👍

rules/aws_db_instance_invalid_engine.go

// Link returns the rule reference link
func (r *AwsDBInstanceInvalidEngineRule) Link() string {
return project.ReferenceLink(r.Name())
Copy link
Member

Choose a reason for hiding this comment

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

This rule doesn't have documentation, so it's better to return an empty string instead.

By the way, the aws_db_instance_invalid_type rule also returns a link, but since there is no documentation, it seems that this should also return an empty string... 😓

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understand, cool 🆒
https://github.com/terraform-linters/tflint-ruleset-aws/blob/master/project/main.go#L10
https://github.com/terraform-linters/tflint-ruleset-aws/tree/v0.2.1/docs/rules

I will add a document for the rule 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed 👍 6a9575c
How about this?

@chaspy chaspy force-pushed the chaspy/lint-rds-engine branch from 5d47eee to 6a9575c Compare February 9, 2021 01:18
@chaspy chaspy requested a review from wata727 February 9, 2021 15:27
wata727
wata727 reviewed Feb 9, 2021
View reviewed changes
docs/rules/README.md Outdated
@@ -52,6 +53,7 @@ These rules warn of possible errors that can occur at `terraform apply`. Rules m
These rules suggest to better ways.

- [aws_instance_previous_type](aws_instance_previous_type.md)
- [aws_db_instance_invalid_engine](aws_db_instance_invalid_engine.md)
Copy link
Member

Choose a reason for hiding this comment

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

It's listed above as possible errors, so we don't need to list it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed. 1a8a656

@chaspy
Fix position
1a8a656 
aws_db_instance_invalid_engine is rule that
warn of possible errors that can occur at `terraform apply`.
So it should be in Possible Errors section.
@chaspy chaspy requested a review from wata727 February 9, 2021 16:41
@chaspy
Copy link
Contributor Author

chaspy commented Feb 9, 2021

Ah, maybe need to add here?

tflint-ruleset-aws/rules/provider.go

Line 9 in 6a9575c

var Rules = append([]tflint.Rule{

@chaspy
Copy link
Contributor Author

chaspy commented Feb 9, 2021

Added some fixes. and I tested in my local.

$ cat main.tf
resource "aws_db_instance" "default" {
  allocated_storage    = 20
  storage_type         = "gp2"
  engine               = "mysql57"
  engine_version       = "5.7"
  instance_class       = "db.t2.micro"
  name                 = "mydb"
  username             = "foo"
  password             = "foobarbaz"
}
$ tflint
1 issue(s) found:

Error: "mysql57" is invalid engine. (aws_db_instance_invalid_engine)

  on main.tf line 4:
   4:   engine               = "mysql57"

Reference: https://github.com/terraform-linters/tflint-ruleset-aws/blob/v0.2.1/docs/rules/aws_db_instance_invalid_engine.md

👍

Ready for the review again!

wata727
wata727 approved these changes Feb 11, 2021
View reviewed changes
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.

👍

@wata727 wata727 merged commit 6893c81 into terraform-linters:master Feb 11, 2021
@chaspy chaspy deleted the chaspy/lint-rds-engine branch February 11, 2021 14:36
chaspy added a commit to chaspy/tflint-ruleset-aws that referenced this pull request Feb 12, 2021
@chaspy
Add document for aws_db_instance_invalid_type
d4b32c8 
ref: terraform-linters#61 (comment)
@chaspy chaspy mentioned this pull request Feb 12, 2021
Merged
wata727 pushed a commit that referenced this pull request Feb 13, 2021
@chaspy
Add document for aws_db_instance_invalid_type (#63)
b71dcb6 
ref: #61 (comment)

Co-authored-by: kondo takeshi <chaspy@users.noreply.github.com>
@circa10a circa10a mentioned this pull request Feb 19, 2021
Closed
@PatMyron
Copy link
Contributor

Wish these could also be pulled from the SDK :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wata727 wata727 wata727 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chaspy @PatMyron @wata727