-
Notifications
You must be signed in to change notification settings - Fork 499
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.
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
Fix create_tidb_cluster_release variable does not work #1062
Conversation
Signed-off-by: Aylei <rayingecho@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for the format issue.
@@ -22,6 +23,7 @@ data "helm_repository" "pingcap" { | |||
} | |||
|
|||
resource "helm_release" "tidb-cluster" { | |||
count = var.create ? 1 : 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The '=' should be aligned with others in the same block.
Same comments for the other updates in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add Terraform fmt
and validate
to CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already an issue tracking this #823
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there's a blank line between, then the =
don't have to be aligned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cherry pick to release-1.0 in PR #1066 |
Signed-off-by: Aylei rayingecho@gmail.com
What problem does this PR solve?
close #1057
Check List
Tests
Related changes
Does this PR introduce a user-facing change?:
@DanielZhangQD @tennix @cofyc PTAL