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

Added additional option for the provider to fix the issue of printing the generated string in logs. #36

Closed
wants to merge 1 commit into from

Conversation

dpmerron-ltd
Copy link

@dpmerron-ltd dpmerron-ltd commented Jun 12, 2018

This PR should hopefully address Issue #17 by adding an additional type 'password' which hides the output of the ID. Possibly a better solution than PR #18 as it won't break any existing terraform code that is referencing the random_string.id..?

This would be a good fix for us, and there is quite a lot of chat on the internet about random_string exposing the string.

Let me know thoughts?

@dpmerron-ltd
Copy link
Author

dpmerron-ltd commented Jun 12, 2018

Acceptance test output:

$ make testacc TEST=./random TESTARGS='-run=TestAccResource'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./random -v -run=TestAccResource -timeout 120m
=== RUN   TestAccResourceID
--- PASS: TestAccResourceID (0.03s)
=== RUN   TestAccResourceIntegerBasic
=== RUN   TestAccResourceIntegerUpdate
=== RUN   TestAccResourceIntegerSeedless_to_seeded
=== RUN   TestAccResourceIntegerSeeded_to_seedless
=== RUN   TestAccResourcePassword
--- PASS: TestAccResourcePassword (0.04s)
=== RUN   TestAccResourcePet_basic
--- PASS: TestAccResourcePet_basic (0.02s)
=== RUN   TestAccResourcePet_length
--- PASS: TestAccResourcePet_length (0.02s)
=== RUN   TestAccResourcePet_prefix
--- PASS: TestAccResourcePet_prefix (0.02s)
=== RUN   TestAccResourcePet_separator
--- PASS: TestAccResourcePet_separator (0.02s)
=== RUN   TestAccResourceShuffle
--- PASS: TestAccResourceShuffle (0.03s)
=== RUN   TestAccResourceString
--- PASS: TestAccResourceString (0.04s)
--- PASS: TestAccResourceIntegerBasic (0.04s)
--- PASS: TestAccResourceIntegerSeedless_to_seeded (0.06s)
--- PASS: TestAccResourceIntegerSeeded_to_seedless (0.06s)
--- PASS: TestAccResourceIntegerUpdate (0.06s)
PASS
ok  	github.com/terraform-providers/terraform-provider-random/random	0.309s```

@dpmerron-ltd
Copy link
Author

dpmerron-ltd commented Jun 12, 2018

This PR should hopefully address Issue #17 by adding an additional type 'password' which hides the output of the ID. Possibly a better solution than PR #18 as it won't break any existing terraform code that is referencing the random_string.id..?

This would be a good fix for us, and there is quite a lot of chat on the internet about random_string exposing the string.

Let me know thoughts?

@dpmerron-ltd dpmerron-ltd changed the title Added additional provider for random_password to fix the issue of ass… Added additional option for the provider to fix the issue of printing the generated string in logs. Jun 12, 2018
@dpmerron-ltd
Copy link
Author

dpmerron-ltd commented Jun 12, 2018

FYI, the output will look as follows;

main.tf

resource "random_password" "password" {
  length = 16
  special = true
  override_special = "/@\" "
}

resource "random_string" "string" {
  length = 16
  special = true
  override_special = "/@\" "
}

output "password" {
  value = "${random_password.password.result}"
}

output "string" {
  value = "${random_string.string.result}"
}

Terraform Apply

terraform apply
random_string.string: Refreshing state... (ID: 1VxT04gjjAhIksis)
random_password.password: Refreshing state... (ID: none)

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

password = EaI3Jdar3X4bqePG
string = 1VxT04gjjAhIksis

@Centre-for-DevOps
Copy link

looks ok

@dpmerron-ltd
Copy link
Author

@apparentlymart thoughts?

@dpmerron-ltd
Copy link
Author

What have I got to do to get a review on this @phinze @apparentlymart @jefferai @jbardin

@jbardin
Copy link
Member

jbardin commented Jul 10, 2018

hio @danmerron-sig,

Sorry about the delay here. This provider is maintained by the core team, and we have been quite distracted by the large chunk of work going on for 0.12.

I added reviewers directly to this PR to help prevent it from getting lost in the shuffle, and we can take a look at it shortly.

@mildwonkey
Copy link
Contributor

mildwonkey commented Jul 19, 2018

Hi @danmerron-sig,

Do you think it would be beneficial to refactor CreateString to call the CreatePassword function then set the id, instead of maintaining mostly duplicate functions? I haven't done a line-by-line comparison but they appear to be the same.

(I should clarify that this is a genuine question; I am not as familiar with providers as I could be)

@dpmerron-ltd
Copy link
Author

@mildwonkey You could be right - however I think the problem is making sure that we don't create a breaking change to get this merged. There was another PR that solved this in a much more elegant way however it would most likely break a lot of user code!

@mildwonkey
Copy link
Contributor

Hi @dpmerron-ltd!

Thank you again for the work you've done here. I am going to close this PR in favor of #18; the next release of the random provider (which I am planning to do in the next few days) will be released as a breaking change.

@mildwonkey mildwonkey closed this Aug 13, 2018
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants