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

Random String and Password Import #100

Closed
wants to merge 2 commits into from

Conversation

jahantech
Copy link
Contributor

@jahantech jahantech commented Mar 2, 2020

This PR enables import functionality for random string. Resource state changes are used to determine if the string generation criterias were met.

@crisp2u
Copy link

crisp2u commented Mar 2, 2020

Looks good. Do you need help testing this ?

@jahantech
Copy link
Contributor Author

jahantech commented Mar 3, 2020

I have performed tests with basic as well as special characters based strings. But if you have some spare time to run through some cases that would be great. In terms of unit testing, I can add a test for updateStringResourceAttributes method if required.

@cursedcoder
Copy link

Looks good.

@cursedcoder
Copy link

So far works pretty good:

module.tcp.random_string.db_password: Importing from ID "PgYIYHschNVy0ZeroQMTcq1mEa6WOd"...
module.tcp.random_string.db_password: Import prepared!
  Prepared random_string for import
module.tcp.random_string.db_password: Refreshing state... [id=PgYIYHschNVy0ZeroQMTcq1mEa6WOd]

Import successful!

The resources that were imported are shown above. These resources are now in
your Terraform state and will henceforth be managed by Terraform.

@paultyng
Copy link
Contributor

@jahantech can you add import steps to the acceptance testing? Here is an example in AWS: https://github.com/terraform-providers/terraform-provider-aws/blob/98b8b848ca94031b20c3e626c9d40484e3af80de/aws/resource_aws_iot_topic_rule_test.go#L33-L37

@paultyng paultyng self-assigned this Mar 12, 2020
@jahantech
Copy link
Contributor Author

jahantech commented Mar 16, 2020

@paultyng Thank you for providing the details.

I have added the tests to my local branch, but they are not currently passing because my updateStringResourceAttributes function relies on _, newMinClass := d.GetChange(charClassMin) and when the tests are executed d.GetChange(charClassMin) always returns 0 changes.

I will look into further to see how I can resolve this issue.

Output for reference:

--- FAIL: TestAccResourceString (0.10s)
    testing.go:569: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) (len=4) {
         (string) (len=9) "min_lower": (string) (len=1) "0",
         (string) (len=11) "min_numeric": (string) (len=1) "0",
         (string) (len=11) "min_special": (string) (len=1) "0",
         (string) (len=9) "min_upper": (string) (len=1) "0"
        }


        (map[string]string) (len=4) {
         (string) (len=9) "min_lower": (string) (len=1) "2",
         (string) (len=11) "min_numeric": (string) (len=1) "4",
         (string) (len=11) "min_special": (string) (len=1) "1",
         (string) (len=9) "min_upper": (string) (len=1) "3"
        }

@paultyng
Copy link
Contributor

paultyng commented Mar 16, 2020

For an import, your code only runs through the import and read functions, the config file is not referenced, so you have a few options:

  • make 0 work somehow
  • add the additional data to the import string in addition to the password/string itself
  • ignore configuration/behavioral fields in the import test

Hard for me to say which is the appropriate choice, I'd have to dig in to the functionality a little more, but wanted to give you some ideas.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 18, 2020

CLA assistant check
All committers have signed the CLA.

@jahantech
Copy link
Contributor Author

jahantech commented Mar 18, 2020

@paultyng I would have to ignore all the fields as they are set to 0. In such case I have ImportStateVerify to false and the test now passes. Let me know if that is acceptable. Also I noticed the travis build is now failing on the master. My tests do pass locally.

@paultyng paultyng mentioned this pull request Mar 19, 2020
@paultyng
Copy link
Contributor

I reworked some of the code and import testing in #104, there were some other minor issues with the repo so cleaned those up as well (vendoring). For import testing, you should explicitly ignore anything that is only user entered and not determined by the read. Its possible though that this would let people import strings that don't satisfy the create criteria, but I think that's an acceptable situation if they already have an existing password or something they want to use.

@paultyng paultyng closed this Mar 19, 2020
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 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants