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

Update AWS integration endpoints #274

Merged
merged 5 commits into from
Oct 23, 2019
Merged

Conversation

mzneos
Copy link
Contributor

@mzneos mzneos commented Sep 30, 2019

Add a few missing AWS integration endpoints

Copy link
Collaborator

@zippolyte zippolyte left a comment

Choose a reason for hiding this comment

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

A few nits on the docstrings. I realize it's the case for all the other accessors, but let's start to be accurate 🙂

return false
}

// SetLambdaARN allocates a new i.LambdaARN and returns the pointer to it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't allocate anything nor return anything.

Suggested change
// SetLambdaARN allocates a new i.LambdaARN and returns the pointer to it.
// SetLambdaARN set LambdaARN to point to the given string.

return false
}

// SetAccountID allocates a new i.AccountID and returns the pointer to it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't allocate anything nor return anything.

Suggested change
// SetAccountID allocates a new i.AccountID and returns the pointer to it.
// SetAccountID set AccountID to point to the given string.

return false
}

// SetLambdaARN allocates a new i.LambdaARN and returns the pointer to it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't allocate anything nor return anything.

Suggested change
// SetLambdaARN allocates a new i.LambdaARN and returns the pointer to it.
// SetLambdaARN set LambdaARN to point to the given string.

return false
}

// SetAccountID allocates a new i.AccountID and returns the pointer to it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't allocate anything nor return anything.

Suggested change
// SetAccountID allocates a new i.AccountID and returns the pointer to it.
// SetAccountID set AccountID to point to the given string.

return false
}

// SetAccountID allocates a new i.AccountID and returns the pointer to it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't allocate anything nor return anything.

Suggested change
// SetAccountID allocates a new i.AccountID and returns the pointer to it.
// SetAccountID set AccountID to point to the given string.

Copy link
Contributor

@gzussa gzussa left a comment

Choose a reason for hiding this comment

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

The integrations.go file is starting to be crowded.
Could we use a Facade pattern to delegate service integrations to their own files? I am not sure if we can do so without breaking backward compatibility.
Also, maybe we should add unit tests to make sure object struct match jsons returned by the API.

datadog-accessors.go Show resolved Hide resolved
datadog-accessors.go Show resolved Hide resolved
datadog-accessors.go Show resolved Hide resolved
datadog-accessors.go Show resolved Hide resolved
datadog-accessors.go Show resolved Hide resolved
@mzneos
Copy link
Contributor Author

mzneos commented Oct 18, 2019

A few nits on the docstrings. I realize it's the case for all the other accessors, but let's start to be accurate 🙂

this was generated by the command make generate so I will try to see if I can fix it directly there (if this is even possible), so that we don't have to do this again

@zippolyte
Copy link
Collaborator

Okay since this is all autogenerated, changing this is not for the scope of this PR IMO.
Let's leave it like this for now.

Copy link
Collaborator

@zippolyte zippolyte left a comment

Choose a reason for hiding this comment

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

Approving, and tagging @bkabrda for a final review and merge

@mzneos
Copy link
Contributor Author

mzneos commented Oct 21, 2019

Okay since this is all autogenerated, changing this is not for the scope of this PR IMO.
Let's leave it like this for now.

Indeed, you're right. Thank you for the review

Copy link
Collaborator

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

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

Hey, thanks a lot for this PR! I only have one minor comment that should be addressed and then we can go ahead and merge this.

integrations.go Outdated Show resolved Hide resolved
@mzneos mzneos requested a review from bkabrda October 22, 2019 11:52
Copy link
Collaborator

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

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

LGTM now, merging. Thanks a lot for your PR!

@bkabrda bkabrda merged commit b4a470c into zorkian:master Oct 23, 2019
kazz187 pushed a commit to kazz187/go-datadog-api that referenced this pull request Nov 28, 2019
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.

4 participants