Support for Dashboard Timeframe #21
Conversation
| @@ -6,11 +6,12 @@ import ( | |||
| "strings" | |||
| ) | |||
|
|
|||
| func (c *SquaredUpClient) CreateDashboard(displayName string, workspaceId string, dashboardContent string) (*Dashboard, error) { | |||
| func (c *SquaredUpClient) CreateDashboard(displayName string, workspaceId string, timeframe string, dashboardContent string) (*Dashboard, error) { | |||
There was a problem hiding this comment.
How does adding a required argument (timeframe) impact upgrades of Terraform provider versions? e.g. user upgrades to this version of provider and their Terraform deployment breaks as now they need to provide an extra string argument.
There was a problem hiding this comment.
timeframe is not a required argument. Its optional in internal/provider/resource_dashboard.go
| @@ -86,6 +86,7 @@ type Dashboard struct { | |||
| Group string `json:"group,omitempty"` | |||
There was a problem hiding this comment.
We don't want to advertise Group (it was an old thing left over from Beacon days)
| @@ -86,6 +86,7 @@ type Dashboard struct { | |||
| Group string `json:"group,omitempty"` | |||
| Name string `json:"name"` | |||
| SchemaVersion string `json:"schemaVersion"` | |||
There was a problem hiding this comment.
We don't want to advertise SchemaVersion (it's an internal thing used for dashboard migrations). Perhaps we need to strip SchemaVersion and Group from Dashobard reads (GETs) too.
| @@ -36,6 +38,7 @@ type squaredupDashboard struct { | |||
| DashboardTemplate jsontypes.Normalized `tfsdk:"dashboard_template"` | |||
| TemplateBindings jsontypes.Normalized `tfsdk:"template_bindings"` | |||
| DashboardContent jsontypes.Normalized `tfsdk:"dashboard_content"` | |||
| Timeframe types.String `tfsdk:"timeframe"` | |||
| Group types.String `tfsdk:"group"` | |||
| Name types.String `tfsdk:"name"` | |||
| SchemaVersion types.String `tfsdk:"schema_version"` | |||
There was a problem hiding this comment.
Ditto previous comment on SchemaVersion and Group
| @@ -269,6 +292,7 @@ func (r *DashboardResource) Update(ctx context.Context, req resource.UpdateReque | |||
| DashboardTemplate: plan.DashboardTemplate, | |||
| TemplateBindings: plan.TemplateBindings, | |||
| DashboardContent: jsontypes.NewNormalizedValue(updatedDashboard), | |||
| Timeframe: types.StringValue(dashboard.Timeframe), | |||
| Group: types.StringValue(dashboard.Group), | |||
| Name: types.StringValue(dashboard.Name), | |||
| SchemaVersion: types.StringValue(dashboard.SchemaVersion), | |||
There was a problem hiding this comment.
Ditto previous comments on SchemaVersion and Group (both fields are referened in a few other places in this repo I haven't commented on too as not changed in this PR)
There was a problem hiding this comment.
I will do a cleanup of these based on our new API documentation. I have raised a JIRA for this
There was a problem hiding this comment.
I will do a cleanup of these based on our new API documentation. I have raised a JIRA for this
Thanks @shaswot77
| "timeframe": schema.StringAttribute{ | ||
| Description: "The timeframe of the dashboard. It should be one of the following: last1hour, last12hours, last24hours, last7days, last30days, thisMonth, thisQuarter, thisYear, lastMonth, lastQuarter, lastYear", | ||
| Optional: true, | ||
| Computed: true, |
There was a problem hiding this comment.
Why is timeframe marked as Computed (in fact lots of attribs are - obviously doesn't mean what I think it does)?
There was a problem hiding this comment.
Its to handle null value as its marked as optional.
There was a problem hiding this comment.
Its to handle null value as its marked as optional.
Ah, ta, so what value will it compute when null?
There was a problem hiding this comment.
just an empty string "". Because terraform store states based on response it expect some value than null
Description
Added support for dashboard time frame
Checklist:
bug-fix,feature, orenhancement