-
Notifications
You must be signed in to change notification settings - Fork 118
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 import for random_string & random_password resource #256
fix import for random_string & random_password resource #256
Conversation
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.
Hi @wakeful and thank you for submitting this PR.
Overall the changes look good.
Would you be able to:
- Rebase on main
- Amend the tests (see below)
- Update the
CHANGELOG.md
- Amend the comments in
/examples/resources/random_password/import.sh
and/examples/resources/random_string/import.sh
to reflect the change in behaviour and regenerate the docs (i.e.,make generate
)
Tests
TestAccResourcePasswordBasic
Remove all items except bcrypt_hash
from ImportStateVerifyIgnore
ImportStateVerifyIgnore: []string{"bcrypt_hash"},
TestAccResourceString
Remove ImportStateVerifyIgnore
a004688
to
3808cf3
Compare
hi @bendbennett as requested:
|
Hi @wakeful thank you for making those updates. I think it might be useful to update the comments in |
3808cf3
to
327b8da
Compare
hey @bendbennett import doc where updated, could you help me fix the broken test? |
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.
Think the changes in importPasswordFunc
and importStringFunc
should fix the issues with the tests.
Have left a couple of other minor suggestions.
Thanks for your continuing input on this.
241235c
to
3e61088
Compare
thanks for your suggestions 👍 |
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.
@wakeful looks good. Thank you for making the changes.
Only comment I have is to alter the CHANGELOG.md
to reflect that these changes are not yet released.
thanks for you help @bendbennett |
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.
Left a suggestion, but overall I think this is 👍
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 initially approved, but then thought of a potential issue where a value set manually before import, would end up overridden by .Default
.
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.
@wakeful I've added comments to the import.sh
for random_password and random_string to try and document the behaviour during import.
71804a5
to
4997b53
Compare
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.
@wakeful I've attached a couple of templates for use in generating docs relating to imports - resources.zip.
If you expand this and drop the resources directory into the /templates
dir and run make generate
it should update password.md
and string.md
with markdown that'll be more practitioner-friendly.
@wakeful are you happy to adopt the last set of suggested changes? |
4997b53
to
7e6267e
Compare
hey @bendbennett all updated please check again - sorry for the delays, I needed to concentrate on my day to day job. |
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.
@wakeful thank you for your continuing efforts on this PR.
I think we're getting close now.
822707c
to
ef0bc4a
Compare
…ing all defaults into TF state Co-authored-by: Benjamin Bennett <[email protected]>
b7acc22
to
74c568b
Compare
Hi @bendbennett I think all is done now :) |
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.
@wakeful nice job! Think we're there. Thank you for your work on this.
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. |
What's the problem
when I try to use
terraform import
withrandom_string
orrandom_password
resourceterraform
is always trying to recreate the object,terraform.tfstate
state file contains null elements instead the default ones.this problem was also reported by other - #106 and #119
How to reproduce?
Create a new
.tf
file with contentand run
import
statement e.q.Result?
running
plan
is trying to replace the existing objectsExpected result?
terraform.tfstate
without my change