-
Notifications
You must be signed in to change notification settings - Fork 562
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
snap/quotas: followups from previous PR #10198
Conversation
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
Thanks to Pawel for spotting this optimization. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
…ferences This ensures we have consistent error messages here when providing multiple groups and reduces spurious unit test failures in a followup where we provide multiple invalid groups that fail validation as we return on the first invalid group found. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
This eliminates the chance we encounter cycles in the sub-groups and infinitely recurse. We also test for this specifically to ensure that the cycle detection logic actually works. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
048e0fd
to
26c4d53
Compare
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
@anonymouse64 I think https://github.com/snapcore/snapd/pull/10152/files#r621290802 still needs fixing? |
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
This is because we want to return multi-errors here eventually instead, in which case it doesn't matter if we iterate over the groups in non-deterministic order since we will always get the same set of errors back if there are multiple errors in the set of groups provided. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
Thanks to Pawel for spotting this one Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tweaks, looks good, just a few small points to consider, I hope they make sense. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's ok but some comments
…g lists Also change the comments to make clear we are being paranoid about cycle detection before recursing deeper into the tree. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
Thanks to Samuele for the suggestion Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from me
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Addresses comments from #10152.