fix(notices): replace "select=all" with "users=all" #368
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a backport of a fix from the snapd port of pebble notices. The original PR can be found at: canonical/snapd#13630 As snapd lacks a client or cli for notices, this commit also replaces "select" with "users" in the pebble client and cli. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
benhoyt
approved these changes
Feb 25, 2024
There was a problem hiding this comment.
Looks good, thanks! Just for the record, we acknowledge this is a backwards-incompatible change, however, we think it's highly unlikely anyone has started using GET /v1/notices especially with select=all yet, so now's the time to change and break it.
Copying this from the snapd PR for context.
Per discussions with @niemeyer, @pedronis, and @olivercalder, we have decided to rename the select=all query parameter for notices. The word "select" should be used when selecting a particular subset of items from a complete set, but in the case of notices, it was being used to extend the original set of notices to include those from all other users.
It has been proposed to rename select=all to users=all, and since select=all and user-id=1234 were mutually exclusive, it makes sense that a client may choose to use either user-id=1234 or users=all, as these are more clearly related.
benhoyt
pushed a commit
that referenced
this pull request
Feb 28, 2024
This is a backport of a fix from the snapd port of pebble notices. The original PR can be found at: canonical/snapd#13630 As snapd lacks a client or cli for notices, this commit also replaces "select" with "users" in the pebble client and cli. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
benhoyt
added a commit
to benhoyt/juju
that referenced
this pull request
Feb 28, 2024
This Pebble version adds the following since v1.7.1: - canonical/pebble#368: replace "select=all" with "users=all". Strictly speaking this is a breaking change, however, it was a design mistake and we want to fix it as soon as possible. It's almost certainly that no one is using this feature yet. - canonical/pebble#369: avoid acquiring state lock in health check endpoint. This fixes (or is one aspect of the fix for) the issue described in https://bugs.launchpad.net/juju/+bug/2052517, so that the GET /v1/health endpoint returns much quicker even when under load. In addition, this has the side effect of bumping up a few of the golang.org/x packages, as Pebble depends on higher versions.
jujubot
pushed a commit
to juju/juju
that referenced
this pull request
Feb 28, 2024
This Pebble version adds the following since v1.7.1: - canonical/pebble#368: replace "select=all" with "users=all". Strictly speaking this is a breaking change, however, it was a design mistake and we want to fix it as soon as possible. It's almost certainly that no one is using this feature yet. - canonical/pebble#369: avoid acquiring state lock in health check endpoint. This fixes (or is one aspect of the fix for) the issue described in https://bugs.launchpad.net/juju/+bug/2052517, so that the GET /v1/health endpoint returns much quicker even when under load.
jujubot
added a commit
to juju/juju
that referenced
this pull request
Feb 28, 2024
#16981 This Pebble version adds the following since v1.7.1: - canonical/pebble#368: replace "select=all" with "users=all". Strictly speaking this is a breaking change, however, it was a design mistake and we want to fix it as soon as possible. It's almost certainly that no one is using this feature yet. - canonical/pebble#369: avoid acquiring state lock in health check endpoint. This fixes (or is one aspect of the fix for) the issue described in https://bugs.launchpad.net/juju/+bug/2052517, so that the GET /v1/health endpoint returns much quicker even when under load.
jujubot
added a commit
to juju/juju
that referenced
this pull request
Feb 28, 2024
#16982 This Pebble version adds the following since v1.7.1: - canonical/pebble#368: replace "select=all" with "users=all". Strictly speaking this is a breaking change, however, it was a design mistake and we want to fix it as soon as possible. It's almost certainly that no one is using this feature yet. - canonical/pebble#369: avoid acquiring state lock in health check endpoint. This fixes (or is one aspect of the fix for) the issue described in https://bugs.launchpad.net/juju/+bug/2052517, so that the GET /v1/health endpoint returns much quicker even when under load. In addition, this has the side effect of bumping up a few of the golang.org/x packages, as Pebble depends on higher versions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a backport of a fix from the snapd port of pebble notices. The original PR can be found at:
canonical/snapd#13630
As snapd lacks a client or cli for notices, this commit also replaces "select" with "users" in the pebble client and cli.