Skip to content

fix(coderd/healthcheck): add daemon-specific warnings to healthcheck output #11490

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

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Jan 8, 2024

Follow-up to #11393
Part of #10676

  • Sorts provisioner daemons by name ascending in output
  • Adds daemon-specific warnings to healthcheck output
  • Reword some messages

- Sorts provisioner daemons by name ascending in output
- Adds daemon-specific warnings to healthcheck output
- Reword some messages
@johnstcn johnstcn self-assigned this Jan 8, 2024
@johnstcn johnstcn changed the title fix(coderd/healthcheck): scope warnings in ProvisionerDaemonsReport fix(coderd/healthcheck): sort provisioner daemons by name in ProvisionerDaemonsReport Jan 8, 2024
@johnstcn johnstcn requested review from mtojek and mafredri January 8, 2024 13:30
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Left a few comments to address/respond to. Otherwise, it is 👍

@@ -26,7 +27,13 @@ type ProvisionerDaemonsReport struct {
Dismissed bool `json:"dismissed"`
Error *string `json:"error"`

ProvisionerDaemons []codersdk.ProvisionerDaemon `json:"provisioner_daemons"`
Items []ProvisionerDaemonsReportItem `json:"items"`
Copy link
Member

Choose a reason for hiding this comment

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

In theory, this is API breaking, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah in theory, but the modifications were only merged this morning :-)


// Ensure stable order for display and for tests
sort.Slice(daemons, func(i, j int) bool {
return daemons[i].Name < daemons[j].Name
Copy link
Member

Choose a reason for hiding this comment

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

doublecheck: we don't need to sort by other fields? There is no way that the report will contain old entries for provisioners with the same name?

Copy link
Member Author

Choose a reason for hiding this comment

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

The database schema requires each provisioner daemon to have a unique name.

@johnstcn johnstcn changed the title fix(coderd/healthcheck): sort provisioner daemons by name in ProvisionerDaemonsReport fix(coderd/healthcheck): add daemon-specific warnings to healthcheck output Jan 8, 2024
@johnstcn johnstcn merged commit 93cf5dc into main Jan 8, 2024
@johnstcn johnstcn deleted the cj/provisioner-healthcheck-fixes branch January 8, 2024 13:55
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants