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(cli): add CLI commands for notices (notices, notice, notify) #298

Merged
merged 59 commits into from
Nov 20, 2023

Conversation

benhoyt
Copy link
Contributor

@benhoyt benhoyt commented Sep 6, 2023

This adds the CLI commands for Pebble Notices (see spec JU048).

The new commands are:

  • pebble notices [--type=<type> --key=<key> --timeout=<duration>]: list (or wait for) notices
  • pebble notice <id-or-type> [<key>]: fetch details about a single notice by ID or type+key
  • pebble notify [--repeat-after=<duration>] <key> [<name=value>...]: record a client ("custom") notice

The PR also updates the existing pebble okay command to acknowledge notices (as well as warnings).

I've also added a section to the README explaining notices (including concepts like "repeated") and the associated commands.

Copy link
Contributor

@flotter flotter left a comment

Choose a reason for hiding this comment

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

Looks great. Some comments about user understanding.

internals/cli/cmd_notices.go Outdated Show resolved Hide resolved
internals/cli/cmd_notices.go Outdated Show resolved Hide resolved
internals/cli/cmd_notices.go Outdated Show resolved Hide resolved
@benhoyt benhoyt added the High Priority Look at me first label Oct 17, 2023
}
}

last, err := lastWarningTimestamp()
Copy link
Contributor Author

@benhoyt benhoyt Oct 17, 2023

Choose a reason for hiding this comment

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

With this PR, the code is in a funny in-between state, because warnings haven't actually been implemented in terms of notices yet (that's for a future PR). I've tried to make it work, but warnings aren't actually used anywhere yet in Pebble, so it's not critical for this to be perfect (just before warnings get refactored to be implemented in terms of notices).

Copy link
Contributor

Choose a reason for hiding this comment

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

If the code doesn't exist yet, what about unifying both inside a single file already, on the notices side since this is the more general concept? Per earlier note, there will already be more complexity involved due to how pebble isn't a singleton on the system, so we migtht as well do this only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By both do you mean notices and warnings? I'm not a fan of that, because all our other commands (with the exception of changes and tasks) are each in their own file, and it's harder to find things if they deviate from that (multiple commands in one file).

@benhoyt benhoyt changed the title feat(cli): add CLI commands for notices (notices, ack, notice, notify) feat(cli): add CLI commands for notices (notices, notice, notify) Oct 17, 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, Ben. A number of comments, but most of them trivial.

internals/cli/cmd_help.go Outdated Show resolved Hide resolved
internals/cli/cmd_notice.go Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
internals/cli/cmd_notices.go Outdated Show resolved Hide resolved
internals/cli/cmd_notices.go Outdated Show resolved Hide resolved
internals/cli/cmd_okay.go Outdated Show resolved Hide resolved
}
}

last, err := lastWarningTimestamp()
Copy link
Contributor

Choose a reason for hiding this comment

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

If the code doesn't exist yet, what about unifying both inside a single file already, on the notices side since this is the more general concept? Per earlier note, there will already be more complexity involved due to how pebble isn't a singleton on the system, so we migtht as well do this only once.

internals/daemon/api_notices.go Show resolved Hide resolved
@jnsgruk
Copy link
Member

jnsgruk commented Oct 27, 2023

Note that CI failures are just because this work is on a fork and unrelated to the changes :)

@jnsgruk jnsgruk requested a review from niemeyer October 27, 2023 10:34
Copy link
Contributor

@flotter flotter left a comment

Choose a reason for hiding this comment

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

LGTM

@benhoyt
Copy link
Contributor Author

benhoyt commented Nov 20, 2023

All Gustavo's comments resolved, now merging.

@benhoyt benhoyt merged commit 010e042 into canonical:master Nov 20, 2023
11 of 16 checks passed
@benhoyt benhoyt deleted the notices-commands branch November 20, 2023 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority Look at me first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants