Skip to content

Eo/fix predicate export#15

Merged
eochoa260 merged 4 commits intomainfrom
eo/fix-predicate-export
Feb 23, 2022
Merged

Eo/fix predicate export#15
eochoa260 merged 4 commits intomainfrom
eo/fix-predicate-export

Conversation

@eochoa260
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread src/cmd/terraform.go Outdated
config = `
%s {
type = "%s"
value = <<-EOT
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Spacing was weird so I had to add spaces to get it to look correct on the export. Not sure if you know what could be causing that.

Comment thread src/cmd/terraform.go Outdated
config = `
%s {
type = "%s"
value = <<-EOT
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm - Whats the reason for adding an if statement here? i guess is there a reason you didn't make the escaping change to line 402? I would think it should be valid no matter the predicate type.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was mainly going off the ticket, it says in there to do this for type == match_regex so I figured we didn't want to do it for the other types. I agree that it would be cleaner if it also worked for other types so that we can just update the original config definition though.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thats fair - but in that case to reduce the duplication of the template i'd do just extract the specific changes we need - IE:

	config := `
  %s {
    type = "%s"
    value = %s
  }
`
	if value != nil {
		predicateValue := ""
		if value.Type == "matches_regex" {
			predicateValue = fmt.Sprint(`<<-EOT
%s
EOT`, value.Value)
		} else {
			predicateValue = fmt.Sprint(`"%s"`, strings.ReplaceAll(value.Value, "\"", "\\\""))
		}
		return templateConfig(config, key, value.Type, predicateValue)
	}
	return ""

but i'm not sure we actually need to have the if statement - i leave it up to you - if we do go without the if statement and "value" is always escaped multiline string we should make sure we do extensive testing of importing and exporting and using terraform to manage the checks with multiple different kinds of predicate types so we can ensure we are not breaking anything.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So I was actually just testing this and it seems to work fine with other types on the export and terraform plan but I haven't applied the terraform to see what the end state looks like. Code looks cleaner without the if statement though so let me apply and make sure that the resources get created as expected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tested applying and it seems to be working as expected for other types too

Copy link
Copy Markdown
Collaborator

@rocktavious rocktavious left a comment

Choose a reason for hiding this comment

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

LGTM

@eochoa260 eochoa260 merged commit 55253d0 into main Feb 23, 2022
@eochoa260 eochoa260 deleted the eo/fix-predicate-export branch February 23, 2022 16:53
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.

2 participants