-
Notifications
You must be signed in to change notification settings - Fork 75
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
api: u.pro.services.disable.v1 #3033
Conversation
Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference) GitHub Issues: No GitHub issues are fixed by this PR. (No commits have Fixes: #### references) Launchpad Bugs: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references) Documentation: The changes in this PR do not require documentation changes. 👍 this comment to confirm that this is correct. |
🌎 This PR changes translatable messages. 🌏 Please select which scenarios apply. For further explanation, please read our policy on message changes.
|
91eb2ba
to
64ec117
Compare
64ec117
to
1e7225b
Compare
ent_cls = entitlements.entitlement_factory(cfg=cfg, name=options.service) | ||
entitlement = ent_cls( | ||
cfg, | ||
assume_yes=True, | ||
called_name=options.service, | ||
purge=options.purge, | ||
) |
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.
Let's verify if the user provided an invalid service
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.
The exception already raised looks pretty good and I added it to the integration test. It looks like this:
{
"code": "entitlement-not-found",
"meta": {
"entitlement_name": "invalid"
},
"title": "could not find entitlement named \"invalid\""
}
|
||
enabled_services_after = _enabled_services_names(cfg) | ||
|
||
progress.finish() |
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.
Should we update the status cache and send the activity ping to the CS here?
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.
In standup we decided:
yes update status cache
no don't send activity ping (it will send on a timer anyway)
1e7225b
to
281cc9d
Compare
@lucasmoura updated! |
281cc9d
to
a47b624
Compare
CI errors are unrelated |
Why is this needed?
This is based on #3015 so that should be merged first.
This introduces u.pro.services.disable.v1 but does not refactor the cli disable command to use it.
Test Steps
New tests should pass
Checklist
Does this PR require extra reviews?