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 GCP integration #285

Merged
merged 7 commits into from
Nov 26, 2019
Merged

Conversation

mzneos
Copy link
Contributor

@mzneos mzneos commented Nov 14, 2019

What does this PR do?

  • Adds the field AutoMute to the IntegrationGCPCreateRequest object.
  • Updates the IntegrationGCP object
  • Refactors the UpdateIntegrationGCP function as it was not working anymore (the endpoints does not seem to work)
    • for backward compatibility, the IntegrationGCPUpdateRequest object was just updated instead of using the already existing IntegrationGCPCreateRequest object, which is identical
  • Updates the integration/integration_test.go tests functions for the GCP integration, adding checks for the AutoMute field

@gzussa gzussa requested review from bkabrda and nmuesch November 21, 2019 14:52
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.

I double checked omitempty. Seems backward compatible. Please do test this manually before to merge it.

@bkabrda
Copy link
Collaborator

bkabrda commented Nov 22, 2019

Hey, thanks for sending this PR. The code looks good, but I'd like to ask you to extend the test TestIntegrationGCPUpdate in integration/integration_test.go. It'd be awesome if that tested most (or ideally all) of the structure fields of IntegrationGCPUpdateRequest.

@mzneos
Copy link
Contributor Author

mzneos commented Nov 22, 2019

Hey, thanks for sending this PR. The code looks good, but I'd like to ask you to extend the test TestIntegrationGCPUpdate in integration/integration_test.go. It'd be awesome if that tested most (or ideally all) of the structure fields of IntegrationGCPUpdateRequest.

Yes of course, I can do that. I'll look into it as soon as I have some time.

@mzneos
Copy link
Contributor Author

mzneos commented Nov 22, 2019

I think the tests should be ok now. I also updated the test for TestIntegrationGCPCreateAndDelete to check the new AutoMute field added.
All the fields returned by ListIntegrationGCP are tested now, in both functions

@bkabrda
Copy link
Collaborator

bkabrda commented Nov 26, 2019

I'm getting the following failure:

=== RUN   TestIntegrationGCPUpdate
--- FAIL: TestIntegrationGCPUpdate (4.80s)
    integrations_test.go:431: Retrieving a GCP integration failed when it shouldn't: json: cannot unmarshal bool into Go struct field IntegrationGCP.automute of type string

It seems that you provided type *string for IntegrationGCP.AutoMute, but it should be *bool.

@mzneos
Copy link
Contributor Author

mzneos commented Nov 26, 2019

Indeed there was a typo in the IntegrationGCP struct. Thank you for spotting that!
The tests should succeed now

@bkabrda
Copy link
Collaborator

bkabrda commented Nov 26, 2019

Perfect, LGTM now => merging. Thank you very much for this contribution!

@bkabrda bkabrda merged commit 8fb1848 into zorkian:master Nov 26, 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.

3 participants