Skip to content
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

Tolerate white-spaces on finish line beginning #12

Merged
merged 5 commits into from
Mar 10, 2020

Conversation

pdecat
Copy link
Contributor

@pdecat pdecat commented Feb 26, 2020

This relaxes finish line matching to support for white-spaces at the beginning and should partly address #11 (it still requires the finish line to not contain terraform code).

Maybe this could go a bit further and remove the spaces at the beginning of the matched finish line.

@katbyte
Copy link
Owner

katbyte commented Feb 28, 2020

Hey @pdecat,

Thanks for this PR. I want to accept it but we (azurerm) actually rely on that behavior to enforce a consistent formatting for our acceptance tests so doing a pattern match is not ideal. However what we could do is expose a flag allowing for custom start/end strings to be passed in, maybe something like terrafmt -blockstart ' `,'?

WDYT

@pdecat
Copy link
Contributor Author

pdecat commented Feb 28, 2020

Hi @katbyte, wouldn't removing the spaces at the beginning of the matched finish line address this?

@pdecat
Copy link
Contributor Author

pdecat commented Feb 28, 2020

Note: I too want to enforce a consistent formatting for our zabbix provider acceptance tests aligned on the official providers.

It's just that currently, terrafmt skips most of them because of those spaces.

@katbyte
Copy link
Owner

katbyte commented Feb 29, 2020

@pdecat, yep, if you remove the spaces at the of the start of that last line it should work. in azurerm everything is:

	return fmt.Sprintf(`
%s

data "azurerm_kubernetes_cluster" "test" {
  name                = "${azurerm_kubernetes_cluster.test.name}"
  resource_group_name = "${azurerm_kubernetes_cluster.test.resource_group_name}"
}
`, r)

One quirk of terrafmt is it will not format the start or finish lines and doesn't consider them part of the tf block. Mainly because it would be much harder and we start having to think of a byte stream rather then a list of lines. For azurerm we did mass find/replaces in GoLand to fix a lot of issue where the closing quote is not on its own line.

@pdecat pdecat force-pushed the tolerate_whitespaces_on_finish branch from 488210b to eb0f97b Compare March 2, 2020 16:31
@pdecat
Copy link
Contributor Author

pdecat commented Mar 2, 2020

I've updated the PR to remove the leading spaces on the finish lines:

# git reset --hard
HEAD is now at 2962e7f Merge pull request #7 from claranet/claranet
# terrafmt fmt zabbix/resource_zabbix_item_test.go
# git diff zabbix/resource_zabbix_item_test.go
diff --git zabbix/resource_zabbix_item_test.go zabbix/resource_zabbix_item_test.go
index 54a2016..51d533b 100644
--- zabbix/resource_zabbix_item_test.go
+++ zabbix/resource_zabbix_item_test.go
@@ -51,52 +51,52 @@ func TestAccZabbixItem_Basic(t *testing.T) {
 
 func testAccZabbixItemConfig(groupName string, templateName string, itemName string) string {
 	return fmt.Sprintf(`
-		resource "zabbix_host_group" "zabbix" {
-			name = "%s"
-		}
+resource "zabbix_host_group" "zabbix" {
+  name = "%s"
+}
 
-		resource "zabbix_template" "my_zbx_template" {
-			host = "%s"
-			groups = ["${zabbix_host_group.zabbix.name}"]
-			name = "display name %s"
-			description = "description for template %s"
-	  	}
-	  
-		resource "zabbix_item" "my_item1" {
-			name = "%s"
-			key = "bilou.bilou"
-			delay = "34"
-			description = "description for item : %s"
-			trends = "300"
-			history = "25"
-			host_id = "${zabbix_template.my_zbx_template.id}"
-	  	}
-	`, groupName, templateName, templateName, templateName, itemName, itemName)
+resource "zabbix_template" "my_zbx_template" {
+  host        = "%s"
+  groups      = ["${zabbix_host_group.zabbix.name}"]
+  name        = "display name %s"
+  description = "description for template %s"
+}
+
+resource "zabbix_item" "my_item1" {
+  name        = "%s"
+  key         = "bilou.bilou"
+  delay       = "34"
+  description = "description for item : %s"
+  trends      = "300"
+  history     = "25"
+  host_id     = "${zabbix_template.my_zbx_template.id}"
+}
+`, groupName, templateName, templateName, templateName, itemName, itemName)
 }
 
 func testAccZabbixItemUpdate(groupName string, templateName string, itemName string) string {
 	return fmt.Sprintf(`
-		resource "zabbix_host_group" "zabbix" {
-			name = "%s"
-		}
+resource "zabbix_host_group" "zabbix" {
+  name = "%s"
+}
 
-		resource "zabbix_template" "my_zbx_template" {
-			host = "%s"
-			groups = ["${zabbix_host_group.zabbix.name}"]
-			name = "display name %s"
-			description = "description for template %s"
-	  	}
-	  
-		resource "zabbix_item" "my_item1" {
-			name = "update_%s"
-			key = "update.bilou.bilou"
-			delay = "23"
-			description = "update description for item : %s"
-			trends = "3"
-			history = "2"
-			host_id = "${zabbix_template.my_zbx_template.id}"
-	  	}
-	`, groupName, templateName, templateName, templateName, itemName, itemName)
+resource "zabbix_template" "my_zbx_template" {
+  host        = "%s"
+  groups      = ["${zabbix_host_group.zabbix.name}"]
+  name        = "display name %s"
+  description = "description for template %s"
+}
+
+resource "zabbix_item" "my_item1" {
+  name        = "update_%s"
+  key         = "update.bilou.bilou"
+  delay       = "23"
+  description = "update description for item : %s"
+  trends      = "3"
+  history     = "2"
+  host_id     = "${zabbix_template.my_zbx_template.id}"
+}
+`, groupName, templateName, templateName, templateName, itemName, itemName)
 }
 
 func testAccZabbixItemExists(resource string) resource.TestCheckFunc {

Copy link
Owner

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

@pdecat i like the idea! it makes sense to just fix the finish line if we can. However it might make wanting an option "--fix-finish-line" given that it's working outside the "block" and all the operations are kinda designed with that in mind.

@@ -107,7 +109,7 @@ func (br *Reader) DoTheThing(filename string) error {
l := s.Text() + "\n"

if err := br.LineRead(br, br.LineCount, l); err != nil {
return fmt.Errorf("NB LineRead failed @ %s:%d for %s: %v", br.FileName, br.LineCount, l, err)
return fmt.Errorf("NB LineRead failed @ %s#%d for %s: %v", br.FileName, br.LineCount, l, err)
Copy link
Owner

Choose a reason for hiding this comment

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

This is reverting a PR to change to :s as such could we revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if line == "`)\n" { // acctest
return true
} else if strings.HasPrefix(line, "`,") { // acctest
spaceLeftAccTestMatcher := regexp.MustCompile("^[[:space:]]*`(,|\\)\n)")
Copy link
Owner

Choose a reason for hiding this comment

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

Could we make this a constant somewhere?

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.

@pdecat pdecat force-pushed the tolerate_whitespaces_on_finish branch from eb0f97b to 023b720 Compare March 4, 2020 07:54
@pdecat pdecat force-pushed the tolerate_whitespaces_on_finish branch from 023b720 to 82e8e4f Compare March 4, 2020 08:01
@pdecat
Copy link
Contributor Author

pdecat commented Mar 4, 2020

Great idea! I've added a command line flag to opt in fixing the finish lines! Named it --fix-finish-lines, plural, as a single fmt pass will likely format more than one block.

Copy link
Owner

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @pdecat! LGTM now 🙂

@katbyte katbyte merged commit 12db38d into katbyte:master Mar 10, 2020
@katbyte katbyte added this to the v0.3.0 milestone Mar 10, 2020
@pdecat pdecat deleted the tolerate_whitespaces_on_finish branch March 10, 2020 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants