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.

Sign up for GitHub

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

Jump to bottom

provider/aws: add support for group name and path changes with group update function #3237

Merged
merged 4 commits into from
Nov 17, 2015
Merged

provider/aws: add support for group name and path changes with group update function #3237

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f5267df
add support for group name and path changes with group update function
Sep 15, 2015
9ab5596
will not swallow error on aws iam group update if we get NoSuchEntity…
pgray Oct 5, 2015
6a2d3ea
remove unneeded print function
Oct 31, 2015
8fcc13a
update aws_iam_group tests to check proper paths and name
Oct 31, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions builtin/providers/aws/resource_aws_iam_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ func resourceAwsIamGroup() *schema.Resource {
return &schema.Resource{
Create: resourceAwsIamGroupCreate,
Read: resourceAwsIamGroupRead,
// TODO
//Update: resourceAwsIamGroupUpdate,
Update: resourceAwsIamGroupUpdate,
Delete: resourceAwsIamGroupDelete,

Schema: map[string]*schema.Schema{
Expand All @@ -30,13 +29,11 @@ func resourceAwsIamGroup() *schema.Resource {
"name": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"path": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Default: "/",
ForceNew: true,
},
},
}
Expand All @@ -45,9 +42,10 @@ func resourceAwsIamGroup() *schema.Resource {
func resourceAwsIamGroupCreate(d *schema.ResourceData, meta interface{}) error {
iamconn := meta.(*AWSClient).iamconn
name := d.Get("name").(string)
path := d.Get("path").(string)

request := &iam.CreateGroupInput{
Path: aws.String(d.Get("path").(string)),
Path: aws.String(path),
GroupName: aws.String(name),
}

Expand All @@ -60,9 +58,10 @@ func resourceAwsIamGroupCreate(d *schema.ResourceData, meta interface{}) error {

func resourceAwsIamGroupRead(d *schema.ResourceData, meta interface{}) error {
iamconn := meta.(*AWSClient).iamconn
name := d.Get("name").(string)

request := &iam.GetGroupInput{
GroupName: aws.String(d.Id()),
GroupName: aws.String(name),
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems superfluous; name isn't used outside of this iam.GetGroupInput struct.
Does it serve some purpose I don't see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it made the code clearer.

}

getResp, err := iamconn.GetGroup(request)
Expand Down Expand Up @@ -93,6 +92,30 @@ func resourceAwsIamGroupReadResult(d *schema.ResourceData, group *iam.Group) err
return nil
}

func resourceAwsIamGroupUpdate(d *schema.ResourceData, meta interface{}) error {
if d.HasChange("name") || d.HasChange("path") {
iamconn := meta.(*AWSClient).iamconn
on, nn := d.GetChange("name")
op, np := d.GetChange("path")
fmt.Println(on, nn, op, np)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for debugging? We should probably output the iam.UpdateGroupInput in a [DEBUG] log statement, similar to other areas:

We don't seem to use op either, so we can just use _ here unless you plan on logging it in the before mentioned DEBUG

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I think I meant to remove this haha. I'll do that.

request := &iam.UpdateGroupInput{
GroupName: aws.String(on.(string)),
NewGroupName: aws.String(nn.(string)),
NewPath: aws.String(np.(string)),
}
_, err := iamconn.UpdateGroup(request)
if err != nil {
if iamerr, ok := err.(awserr.Error); ok && iamerr.Code() == "NoSuchEntity" {
d.SetId("")
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a fine thing to do in the Read function, but I don't think this is the best behavior here: if (for some reason) the group gets deleted between the creation of the plan and the application of the plan then this will result in a successful exit but will leave the user with no group resource, even though the config says that the resource should exist. This is confusing, since users will expect the resource to match their config if terraform apply exits without error.

Instead I think it's better to actually fail in this case so we tell the user that it wasn't possible to converge on the settings in the config. If the user then makes a new plan the Read function will detect the resource is gone and the diff will include a create action as expected, allowing Terraform to create a fresh resource.

}
return fmt.Errorf("Error updating IAM Group %s: %s", d.Id(), err)
}
return resourceAwsIamGroupRead(d, meta)
}
return nil
}

func resourceAwsIamGroupDelete(d *schema.ResourceData, meta interface{}) error {
iamconn := meta.(*AWSClient).iamconn

Expand Down
13 changes: 13 additions & 0 deletions builtin/providers/aws/resource_aws_iam_group_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ func TestAccAWSIAMGroup_basic(t *testing.T) {
testAccCheckAWSGroupAttributes(&conf),
),
},
resource.TestStep{
Config: testAccAWSGroupConfig2,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSGroupExists("aws_iam_group.group", &conf),
testAccCheckAWSGroupAttributes(&conf),
),
},
},
})
}
Expand Down Expand Up @@ -105,3 +112,9 @@ resource "aws_iam_group" "group" {
path = "/"
}
`
const testAccAWSGroupConfig2 = `
resource "aws_iam_group" "group" {
name = "test-group2"
path = "/funnypath/"
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this fail the test? Maybe we could supply testAccCheckAWSGroupAttributes with the expected path, and compare them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the testAccCheckAWSGroupAttributes function handle for the expected names and paths. Hope it makes sense.

}
`