-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
providers/aws: Opsworks permission resource #6304
Conversation
} | ||
} | ||
|
||
func resourceAwsOpsworksPermissionDelete(d *schema.ResourceData, meta interface{}) error { |
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.
is there no delete functionality here?
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.
Yes. There is no delete. Also create is not really there, as permissions objects are created on creation of the stack. These are crated with default values an can than be changed.
Hi guys! Happy friday! What's the status of this? @stack72 Anything else you would like to see changed before we can have it merged? |
Morning guys! @stack72 are you still able to take a look at this, or should we see if @apparentlymart has time? |
if awserr.Code() == "ResourceNotFoundException" { | ||
log.Printf("[INFO] Permission not found") | ||
d.SetId("") | ||
d.Set("id", "") |
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.
We don't need this - the SetId("") takes care of it :)
Hi @janschumann Apologies for this slipping through the cracks (thanks @u2mejc for the reminder!) I have just ran the tests (after leaving a few more small comments). The test fails I am afraid
this is because of this line in the test code:
We are not creating that test user as part of the test - once this gets created, they should pass. So close to being ready for merge here! Thanks Paul |
Hi @janschumann Any thoughts on how we get the tests for this resource working as expected? Paul |
Jepp sorry, I have them working already. Hope I will find time for the finish during the next few days. Best, Jan |
Now I was able to interpret what the failing test tells us: The user which is referenced by Sorry for the confusion and the delay here! I will replace |
I will add doc for user_profile later today |
Hi @janschumann Testing this right now :) Paul |
client := meta.(*AWSClient).opsworksconn | ||
|
||
req := &opsworks.CreateUserProfileInput{ | ||
AllowSelfManagement: aws.Bool(d.Get("allow_self_management").(bool)), |
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.
As this is an optional param, with no default, what will happen here if there is no value set?
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.
@stack72 unfortunately the AWS documentation does not mention a default value, but as you can see in the test, the AWS default is false
. Should I add Computed: true
to that parameter?
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 personally would skip the Computed and add Default: false
to the schema. That way we can guarantee that we have a value and d.GetOk will not fail to get a value
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.
check. default value added.
thanks @janschumann will run these tests really soon - just testing something else first! P. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
The code in this PR works, but unfortunately the tests fail, complaining that
aws_opsworks_permission.tf-acc-perm
could not be found.Also I am not sure if I have implemented it correctly. E.g. I had to implement a
Delete
function although this resource cannot be deleted. I left the delete function empty ...