SLO chart type support #549
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #549 +/- ##
=======================================
Coverage 94.87% 94.87%
=======================================
Files 35 35
Lines 2107 2107
=======================================
Hits 1999 1999
Misses 89 89
Partials 19 19 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
recheck |
There was a problem hiding this comment.
I've added comments where i think things could be improved, mostly as nits.
However, I do think adopting the Operation Context methods should be done since it they also provide the ability to gracefully stop resources being created/destroyed during the provider shutdown.
| Create: slochartCreate, | ||
| Read: slochartRead, | ||
| Update: slochartUpdate, | ||
| Delete: slochartDelete, |
There was a problem hiding this comment.
Please us the <Operation>Context methods since these methods are deprecated by the API.
| Read: slochartRead, | ||
| Update: slochartUpdate, | ||
| Delete: slochartDelete, | ||
| Exists: chartExists, |
There was a problem hiding this comment.
The API mentions that this is deprecated and not required if Read is implemented.
| debugOutput, _ := json.Marshal(payload) | ||
| log.Printf("[DEBUG] SignalFx: Create SLO Chart Payload: %s", string(debugOutput)) |
There was a problem hiding this comment.
For this, you could do:
tflog.Debug(ctx, "Created SLO chart", tfext.NewLogFields().JSON("payload", payload))This would keep the code more concise.
| return err | ||
| } | ||
| // Since things worked, set the URL and move on | ||
| appURL, err := buildAppURL(config.CustomAppURL, CHART_APP_PATH+sloChart.Id) |
There was a problem hiding this comment.
You can use the following method to make sure this uri is correctly built.
pmeta.LoadApplicationURL(ctx, meta, ChartAppPath, slochart.Id)| if err := d.Set("slo_id", c.SloId); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
This can just be:
| if err := d.Set("slo_id", c.SloId); err != nil { | |
| return err | |
| } | |
| return nil | |
| return d.Set("slo_id", c.SloId) |
| debugOutput, _ := json.Marshal(c) | ||
| log.Printf("[DEBUG] SignalFx: Got SLO Chart to enState: %s", string(debugOutput)) |
There was a problem hiding this comment.
Considering there is only one field being set, I don't believe this is needed.
|
All of the suggestions are addressed. |
No description provided.