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

feat(daemon): add ErrorResponse to allow external error creation #303

Merged
merged 7 commits into from
Sep 29, 2023

Conversation

flotter
Copy link
Contributor

@flotter flotter commented Sep 11, 2023

#265 provides the ability for the daemon REST API to be extended by appending new handlers to the API list.

For example, the following new reboot command can be added now:

// requestReboot asks the daemon to gracefully shut down the system
// and issue a reboot.
func requestReboot(d *daemon.Daemon) daemon.Response {
        d.HandleRestart(restart.RestartSystem)
        result := deviceResult{}
        return daemon.SyncResponse(result)
}

Note that SyncResponse was already public, which allows the code above to work.

Following this patch, we can now reply with a suitable error response:

:
    switch(...) {
    case reboot:
        requestReboot(...)
    default:    
        return daemon.ErrorResponse(http.StatusBadRequest,"invalid media type %q", mediaType)
    {
:

@flotter flotter changed the title feat(daemon): allow externally supplied HTTP responses feat(daemon): expose HTTP error responses Sep 11, 2023
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me, especially given that SyncResponse and AsyncResponse are already exported.

Copy link
Contributor

@paul-rodriguez paul-rodriguez left a comment

Choose a reason for hiding this comment

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

This is very straightforward

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Some remarks below. Please let me know how it sounds.

internals/daemon/response.go Outdated Show resolved Hide resolved
internals/daemon/response.go Outdated Show resolved Hide resolved
internals/daemon/response.go Outdated Show resolved Hide resolved
internals/daemon/response.go Outdated Show resolved Hide resolved
internals/daemon/response.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
internals/daemon/response.go Outdated Show resolved Hide resolved
@flotter flotter changed the title feat(daemon): expose HTTP error responses feat(daemon): add Responsef() to allow external error creation Sep 22, 2023
client/errors.go Outdated Show resolved Hide resolved
client/errors.go Outdated Show resolved Hide resolved
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Nice refactoring, thanks. Only a few superficial details:

client/client.go Outdated Show resolved Hide resolved
client/errors.go Outdated Show resolved Hide resolved
client/errors.go Outdated Show resolved Hide resolved
internals/daemon/response.go Show resolved Hide resolved
internals/daemon/response.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@anpep anpep left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this, I think those changes are solid!

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

One more detail about the StatusCode conversation above:

client/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Sorry for the negativity here, but I really don't think breaking backwards-compatibility (for these minor adjustments) is worth it, and I also don't think we should import client from daemon. Detailed comments below.

client/errors.go Outdated Show resolved Hide resolved
client/errors.go Outdated Show resolved Hide resolved
client/errors.go Outdated Show resolved Hide resolved
client/errors.go Outdated Show resolved Hide resolved
internals/cli/cli.go Outdated Show resolved Hide resolved
internals/daemon/api_changes_test.go Outdated Show resolved Hide resolved
internals/daemon/response.go Show resolved Hide resolved
internals/daemon/response.go Show resolved Hide resolved
internals/daemon/response.go Outdated Show resolved Hide resolved
internals/daemon/response.go Outdated Show resolved Hide resolved
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks for the changes -- I'm good with this change now!

internals/daemon/response.go Outdated Show resolved Hide resolved
client/files_test.go Outdated Show resolved Hide resolved
internals/daemon/response.go Outdated Show resolved Hide resolved
internals/daemon/response.go Outdated Show resolved Hide resolved
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

This looks much better to me now, and a very lightweight PR.

internals/daemon/response.go Outdated Show resolved Hide resolved
internals/daemon/response.go Show resolved Hide resolved
@benhoyt benhoyt changed the title feat(daemon): add Responsef() to allow external error creation feat(daemon): add ErrorResponse to allow external error creation Sep 28, 2023
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thanks for taking this through, Fred. Looks good.

@flotter flotter merged commit ae456c9 into canonical:master Sep 29, 2023
13 checks passed
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.

5 participants