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 legend_size to api request #421

Merged
merged 3 commits into from
Apr 29, 2020
Merged

Add legend_size to api request #421

merged 3 commits into from
Apr 29, 2020

Conversation

matthewduren
Copy link
Contributor

Closes #420

Copy link
Contributor

@jirikuncar jirikuncar left a comment

Choose a reason for hiding this comment

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

I looks good, however we should probably include a validation as only following values are supported:

  • 2
  • 4
  • 8
  • 16
  • auto

@matthewduren matthewduren force-pushed the master branch 3 times, most recently from 872702e to 0ea4daa Compare February 26, 2020 15:25
@matthewduren
Copy link
Contributor Author

I looks good, however we should probably include a validation as only following values are supported:

  • 2
  • 4
  • 8
  • 16
  • auto

@jirikuncar all set. I considered also adding a test for this, but I don't see other cases where validation is being tested so I didn't add such a test.

@bkabrda
Copy link
Contributor

bkabrda commented Mar 11, 2020

Hey @matthewduren, awesome work! The PR looks ok, but there's one more thing I'd like to request: we recently started recording the interactions with the DD API using "cassettes" to be able to run all tests fast and without internet connection - more details on that in [1]. Could you please rebase on latest master and regenerate cassette for the test you touched with make cassettes TESTARGS="-run TestAccDatadogDashboard_update"? (Do note that this will actually interact with the DD API much like acceptance tests, so make sure you don't provide api/app keys for a production org).

Alternatively, if you don't/can't do this, I'd be happy to add a commit to this PR updating the cassettes, assuming you give me access to your branch from which you did this PR.

[1] https://github.com/terraform-providers/terraform-provider-datadog#developing-the-provider

@matthewduren
Copy link
Contributor Author

Hey @matthewduren, awesome work! The PR looks ok, but there's one more thing I'd like to request: we recently started recording the interactions with the DD API using "cassettes" to be able to run all tests fast and without internet connection - more details on that in [1]. Could you please rebase on latest master and regenerate cassette for the test you touched with make cassettes TESTARGS="-run TestAccDatadogDashboard_update"? (Do note that this will actually interact with the DD API much like acceptance tests, so make sure you don't provide api/app keys for a production org).

Alternatively, if you don't/can't do this, I'd be happy to add a commit to this PR updating the cassettes, assuming you give me access to your branch from which you did this PR.

[1] https://github.com/terraform-providers/terraform-provider-datadog#developing-the-provider

No worries, will update with this addition next week.

@matthewduren
Copy link
Contributor Author

matthewduren commented Mar 16, 2020

I'm not sure what I've done wrong here, but I appear to have missed something, somewhere.

Edit: It may simply be a racey test. I had the import test fail twice locally, then it started working.

@ghost ghost added size/XXL and removed size/XL labels Mar 16, 2020
@nmuesch
Copy link
Contributor

nmuesch commented Mar 18, 2020

Hey @matthewduren I think this PR is pretty much good to go, thanks for working on it! I noticed there is some user data, like email in the recorded cassette. If I could get write access to this branch I'd be happy to create a recording from a dedicated account and commit over the existing recording.

@matthewduren
Copy link
Contributor Author

Ah sure, I wasn't sure so I just setup a demo account. Please force push over that if you can to keep out the clutter. Once you accept the invite I'll make sure you have the appropriate permissions.

@nmuesch
Copy link
Contributor

nmuesch commented Mar 19, 2020

Hey @matthewduren thanks, I just accepted. I'll try to update the recordings tomorrow and get this merged in 🙇

@nmuesch
Copy link
Contributor

nmuesch commented Mar 20, 2020

Hey @matthewduren I wound up pushing to your fork, but it looks like its based on the mater branch. As an alternative, what do you think if we just drop the commits modifying the cassettes, we can merge, and I can update the cassettes in a PR directly after. (Note we're looking into ways of improving this process, so thanks for working on this 😄)

Would you be up for doing that?

@nmuesch
Copy link
Contributor

nmuesch commented Apr 29, 2020

Apologies for my delay here @matthewduren . Going to go ahead and merge this in now and commit the updated cassettes separately. Thanks again for the work here!

@nmuesch nmuesch merged commit cb1a945 into DataDog:master Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing legend_size on datadog_dashboard
5 participants