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

Add AWS Integration #162

Merged
merged 2 commits into from
Aug 5, 2018
Merged

Add AWS Integration #162

merged 2 commits into from
Aug 5, 2018

Conversation

MattHodge
Copy link
Contributor

@MattHodge MattHodge commented Jun 25, 2018

⚠️ I am a new Go developer so lots of feedback is wanted! Be critical, please 🤓

PR Details

Notes:

  • There isn't a way listed in the Datadog API docs to update AWS Accounts added to the AWS integration so there is no Update function.

Questions:

  • Adding an account returns a response like {'external_id': '123456789'}. I am currently marshalling this to the IntegrationAWSAccountCreateResponse struct. It appears external_id is never needed again in further API calls so perhaps I should just return an error from the CreateIntegrationAWS function and delete this struct?
  • Should I add a GetIntegrationAWSByAccount or similar function to prevent a consumer of the client having to loop through the results of the GetIntegrationAWS function?

@ghost ghost self-assigned this Jun 25, 2018
integrations.go Outdated

var awsIntegrations []IntegrationAWSAccount

for _, account := range response.Accounts {

Choose a reason for hiding this comment

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

Is the loop here necessary? Couldn't we just return the response object?

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICT
line 174 until 178 can be replaced with:

return &response.Accounts, nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been updated @ojongerius 👍

@iancward
Copy link

The code in integrations.go looks pretty much exactly like what I had (I was currently working on the test bits).
Regarding the external_id response to the integration create, I'd leave this in place. My intent with opening the issue for this was so that I could add it to the terraform-datadog provider, which would expose the external_id as an attribute to the resource. It could then be consumed by some other code that created the roles on the AWS side.

I did experiment to see if it was possible to perform a GET request for a specific AWS integration; however, it looks like that's not exposed.

@iancward
Copy link

iancward commented Jul 2, 2018

I opened a support ticket to Datadog and confirmed that the API for the AWS integration does not yet support a PUT update action. So we're good on that.

@MattHodge
Copy link
Contributor Author

@yfronto Can you provide some feedback on this PR?

Copy link
Collaborator

@ojongerius ojongerius left a comment

Choose a reason for hiding this comment

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

Address the feedback on line 174 in integrations.go and we are good to go AFAIAC!

integrations.go Outdated

var awsIntegrations []IntegrationAWSAccount

for _, account := range response.Accounts {
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICT
line 174 until 178 can be replaced with:

return &response.Accounts, nil

Copy link
Collaborator

@ojongerius ojongerius left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ojongerius ojongerius merged commit 114bbae into zorkian:master Aug 5, 2018
@ojongerius
Copy link
Collaborator

Released in v2.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants