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 ForceNew to layout_type dashboard attribute #774

Merged
merged 4 commits into from
Dec 8, 2020
Merged

Conversation

zippolyte
Copy link
Contributor

@zippolyte zippolyte commented Dec 7, 2020

Add ForceNew to layout_type dashboard attribute so that users can change the layout type of a dashboard.
Note that changing the type only is not sufficient, a layout property need to be added/removed to/from all the widgets in that dashboard depending on the change. Also a few widgets might be available in one type and not the other, which is another thing to lookout for when changing the layout type

fixes #772

@zippolyte zippolyte requested review from a team as code owners December 7, 2020 14:19
@therve
Copy link
Contributor

therve commented Dec 7, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matt-miller-ddog
Copy link
Contributor

We don't support changing a dashboard's layout type; our API will reject these payloads even if our Terraform provider doesn't.

@zippolyte
Copy link
Contributor Author

That's the thing, it won't try to update the dashboard, it will just delete the old one, and create a new one from scratch

nmuesch
nmuesch previously approved these changes Dec 7, 2020
Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

LGTM, minor question/note about the alert id, but other than that this seems 💯

datadog/resource_datadog_dashboard_test.go Show resolved Hide resolved
datadog/resource_datadog_dashboard_test.go Show resolved Hide resolved
@@ -634,6 +684,130 @@ resource "datadog_dashboard" "free_dashboard" {
}`, uniq)
}

func datadogSimpleFreeDashboardConfig(uniq string) string {
return fmt.Sprintf(`
resource "datadog_dashboard" "simple_free_dashboard" {
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to keep the same resource name in your update, otherwise terraform ends doing a delete/create and you don't test forceNew

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦

@zippolyte
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matt-miller-ddog
Copy link
Contributor

matt-miller-ddog commented Dec 7, 2020

I get the idea behind ForceNew now, but I'm still worried about approving. The issue is with widgets' layout blocks. Traditionally they're screenboard-only, and our API will force users to add/remove them when changing layout types. But the Multi-Size Layout project uses layout in timeboard widgets as well, using the same structure but for different purposes and with different expected values. That means that for orgs with MSL enabled, the API will allow users to switch from free to ordered (or the other way, once MSL goes GA and makes its way into TF) and produce a corrupt / bad-looking board.

(But if we don't do this PR, TF users may end up renaming their resources with new types to accomplish the same thing in a hackier way, which leaves us in the same situation. I'll think this over for now.)

@matt-miller-ddog
Copy link
Contributor

Clarification: if Terraform is able to detect and prevent using layout on TBs + not using layout on SBs and prevent the API calls appropriately, this should be fine; if we're counting on the API to do it then this will become an issue once we start rolling out MSL.

@therve
Copy link
Contributor

therve commented Dec 7, 2020

Clarification: if Terraform is able to detect and prevent using layout on TBs + not using layout on SBs and prevent the API calls appropriately, this should be fine; if we're counting on the API to do it then this will become an issue once we start rolling out MSL.

I suppose terraform could do that, but I'd rather not add more logic if we can avoid it. I'm not exactly sure how it's related to the issue at hand?

IIUC, right now you can either specify ordered, and no layout, or free, and layout everything. If you try to change the type, the API fails. In the future, with MSL, you can specify layout in ordered? And thus the API will allow changing the type? If that's the case, I agree we probably shouldn't merge this.

Copy link
Contributor

@matt-miller-ddog matt-miller-ddog left a comment

Choose a reason for hiding this comment

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

IIUC, right now you can either specify ordered, and no layout, or free, and layout everything. If you try to change the type, the API fails. In the future, with MSL, you can specify layout in ordered? And thus the API will allow changing the type? If that's the case, I agree we probably shouldn't merge this.

The API still won't allow Dashboard.update to change board types even with MSL (meaning that we'd still need ForceNew to do it); but other than that your statement is accurate, and TB payloads will have layout in them to manage MSL layout. As it stands now, the API won't be able to tell the difference between a SB layout vs an MSL TB layout and will allow users to change their resources' layout types without also forcing them to fix their layout values.

I'd classify this as "a future dashboards backend bug that will apply to TF users once MSL is rolled out" (and not merging this reduces its severity but also interferes with this layout_type edit bugfix) so I'll see if dashboards-backend is willing to address this.

At any rate, this future MSL bug will be present for TF users with MSL enabled regardless of whether this PR is merged or not, so I'm willing to approve this PR to fix the existing bug report. I'll raise awareness of this situation so that hopefully we can dodge the future MSL layout bug before it happens.

@zippolyte zippolyte merged commit 24ecc84 into master Dec 8, 2020
@zippolyte zippolyte deleted the hippo/dfn branch December 8, 2020 10:47
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.

Error when updating Dashboard from ordered layout type to free layout type.
4 participants