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

many: rename remainingPermissions to outstandingPermissions #14865

Merged

Conversation

olivercalder
Copy link
Member

@olivercalder olivercalder commented Dec 16, 2024

Rename remainingPermissions to outstandingPermissions and clarify their purpose and behavior in request prompts.

This work is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-30515

@olivercalder olivercalder added the Simple 😃 A small PR which can be reviewed quickly label Dec 16, 2024
@olivercalder olivercalder added this to the 2.68 milestone Dec 16, 2024
@github-actions github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Dec 16, 2024
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

+1 for the last commit here

@olivercalder olivercalder force-pushed the prompting-rename-remainingPermissions branch from 339b2c1 to 148b7ab Compare December 17, 2024 20:58
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.28%. Comparing base (24a0034) to head (6c9cc3f).
Report is 74 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14865      +/-   ##
==========================================
+ Coverage   78.20%   78.28%   +0.08%     
==========================================
  Files        1151     1157       +6     
  Lines      151396   152627    +1231     
==========================================
+ Hits       118402   119491    +1089     
- Misses      25662    25764     +102     
- Partials     7332     7372      +40     
Flag Coverage Δ
unittests 78.28% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@olivercalder olivercalder force-pushed the prompting-rename-remainingPermissions branch from 148b7ab to b162d39 Compare December 18, 2024 17:57
@github-actions github-actions bot removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Dec 18, 2024
@olivercalder olivercalder force-pushed the prompting-rename-remainingPermissions branch from b162d39 to 248985f Compare December 18, 2024 18:24
Copy link
Member

@andrewphelpsj andrewphelpsj left a comment

Choose a reason for hiding this comment

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

Thanks!

@olivercalder olivercalder force-pushed the prompting-rename-remainingPermissions branch from 248985f to 6c9cc3f Compare December 18, 2024 19:18
Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -33,12 +33,12 @@ const (
MaxOutstandingPromptsPerUser = maxOutstandingPromptsPerUser
)

func NewPrompt(id prompting.IDType, timestamp time.Time, snap string, iface string, path string, remainingPermissions []string, availablePermissions []string, originalPermissions []string) *Prompt {
func NewPrompt(id prompting.IDType, timestamp time.Time, snap string, iface string, path string, outstandingPermissions []string, availablePermissions []string, originalPermissions []string) *Prompt {
Copy link
Contributor

Choose a reason for hiding this comment

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

otherwise it barely fits the screen 😄

Suggested change
func NewPrompt(id prompting.IDType, timestamp time.Time, snap string, iface string, path string, outstandingPermissions []string, availablePermissions []string, originalPermissions []string) *Prompt {
func NewPrompt(
id prompting.IDType, timestamp time.Time, snap, iface, path string,
outstandingPermissions, availablePermissions, originalPermissions []string,
) *Prompt {

@@ -435,7 +435,7 @@ var timeAfterFunc = func(d time.Duration, f func()) timeutil.Timer {
//
// The caller must ensure that the given permissions are in the order in which
// they appear in the available permissions list for the given interface.
func (pdb *PromptDB) AddOrMerge(metadata *prompting.Metadata, path string, requestedPermissions []string, remainingPermissions []string, listenerReq *listener.Request) (*Prompt, bool, error) {
func (pdb *PromptDB) AddOrMerge(metadata *prompting.Metadata, path string, requestedPermissions []string, outstandingPermissions []string, listenerReq *listener.Request) (*Prompt, bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (pdb *PromptDB) AddOrMerge(metadata *prompting.Metadata, path string, requestedPermissions []string, outstandingPermissions []string, listenerReq *listener.Request) (*Prompt, bool, error) {
func (pdb *PromptDB) AddOrMerge(
metadata *prompting.Metadata, path string,
requestedPermissions []string, outstandingPermissions []string, listenerReq *listener.Request,
) (*Prompt, bool, error) {

@olivercalder
Copy link
Member Author

Test failures:

  • openstack:debian-12-64:tests/main/install-refresh-remove-hooks:parallel --- openstack timeout problems
  • openstack:debian-12-64:tests/main/microk8s-smoke:edge --- openstack timeout problems
  • openstack:debian-sid-64:tests/main/microk8s-smoke:edge --- openstack timeout problems
  • openstack:debian-sid-64:tests/main/snap-env:regular --- openstack timeout problems
  • openstack:fedora-40-64:tests/main/bare-snapctl --- openstack problems
  • openstack:opensuse-15.5-64:tests/main/microk8s-smoke:edge --- openstack problems
  • openstack:opensuse-15.6-64:tests/main/microk8s-smoke:edge --- openstack problems
  • openstack:opensuse-15.6-64:tests/main/refresh-amend:try_mode --- openstack problems
  • openstack:opensuse-tumbleweed-64:project --- openstack problems
  • google:ubuntu-16.04-64:tests/main/auto-refresh-gating-from-snap --- unrelated, fix in the works tests: ensure chrony syncs time before tests #14888
  • google:ubuntu-16.04-64:tests/main/auto-refresh-pre-download:close --- unrelated, fix in the works tests: ensure chrony syncs time before tests #14888
  • google:ubuntu-16.04-64:tests/main/auto-refresh-pre-download:close_mid_restart --- unrelated, fix in the works tests: ensure chrony syncs time before tests #14888
  • google:ubuntu-16.04-64:tests/main/snapd-state --- unrelated
  • google:ubuntu-20.04-64:tests/unit/go:clang --- unrelated, fix in the works tests: increase timeout for unit tests on focal #14876
  • google-arm:ubuntu-core-22-arm-64:tests/core/snapd-refresh-undo --- unrelated, fix in the works tests: correct snapd revision number in snapd-refresh-undo for ARM #14880

@ndyer ndyer merged commit 08d3c8d into canonical:master Dec 20, 2024
53 of 59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants