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.

Sign up for GitHub

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

Jump to bottom

api: u.pro.services.enable.v1 #3015

Merged
merged 4 commits into from
Apr 5, 2024
Merged

api: u.pro.services.enable.v1 #3015

merged 4 commits into from
Apr 5, 2024

Conversation

orndorffgrant
Copy link
Collaborator

@orndorffgrant orndorffgrant commented Mar 26, 2024 •
edited
Loading

Why is this needed?

This is based on #3028 so that should be merged first.

This adds u.pro.services.enable.v1 with progress support and the code to allow the --show-progress flag for the api subcommand.

This does not yet refactor cli enable to use the api.

Test Steps

New tests should pass.

Checklist

  • I have updated or added any unit tests accordingly
  • I have updated or added any integration tests accordingly
  • Changes here need to be documented, and this was done in:

Does this PR require extra reviews?

  • Yes
  • No

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 26, 2024 •
edited
Loading

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 require documentation changes, but those were not addressed yet.

👍 this comment to confirm that this is correct.

@github-actions GitHub Actions
Copy link

🌎 This PR changes translatable messages. 🌏

Please select which scenarios apply. For further explanation, please read our policy on message changes.

  • New messages are being added.
    • We will ask translators to take a look and add translations if they have time, but it will not block this PR.
  • Existing messages are being changed.
    • ⚠️ Please add a comment with justification of why messages are being altered.
    • If the changes are trivial (e.g. a typo fix), then translations must be preserved.
    • If the changes are substantial, then we will ask translators to take a look and update translations if they have time, but it will not block this PR.
  • Existing messages are being deleted.
    • No special action needed.

@orndorffgrant orndorffgrant force-pushed the api-enable-finally branch 5 times, most recently from 9604c53 to 4f86793 Compare March 27, 2024 13:33
@orndorffgrant orndorffgrant mentioned this pull request Mar 27, 2024
Closed
4 tasks
@orndorffgrant orndorffgrant force-pushed the api-enable-finally branch 10 times, most recently from e42dc41 to 37f2407 Compare March 28, 2024 16:58
@orndorffgrant
Copy link
Collaborator Author

I will separate out the enable cli refactor to use the api into a separate PR, since that is the only part that is blocking this (and is only blocked because a separate internal assume_yes refactor is required that doesn't affect the API).

@orndorffgrant orndorffgrant force-pushed the api-enable-finally branch 2 times, most recently from bd7800d to ab3ebc8 Compare April 1, 2024 18:01
@orndorffgrant orndorffgrant mentioned this pull request Apr 1, 2024
Merged
5 tasks
@orndorffgrant orndorffgrant changed the title wip: Api enable finally api: u.pro.services.enable.v1 Apr 1, 2024
@orndorffgrant orndorffgrant marked this pull request as ready for review April 2, 2024 00:40
@orndorffgrant orndorffgrant mentioned this pull request Apr 2, 2024
Merged
5 tasks
renanrodrigo
renanrodrigo approved these changes Apr 4, 2024
View reviewed changes
Copy link
Member

@renanrodrigo renanrodrigo left a comment

Choose a reason for hiding this comment

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

Thanks for the efforts, this LGTM.
I am most curious on how progress will work for other endpoints but I like the way it was thought and the implementation details here.

uaclient/api/api.py
@@ -124,7 +129,12 @@ def call_api(
)

try:
result = endpoint.fn(options, cfg)
if endpoint.supports_progress:
Copy link
Member

Choose a reason for hiding this comment

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

smart

lucasmoura
lucasmoura reviewed Apr 4, 2024
View reviewed changes
uaclient/api/u/pro/services/enable/v1.py
with lock.RetryLock(
lock_holder="u.pro.services.enable.v1",
):
success, fail_reason = entitlement.enable(progress)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we try to refresh the contract similar to what the CLI does ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed, created #3042 to cover adding this with caching

uaclient/api/u/pro/services/enable/v1.py
Comment on lines +99 to +108
ent_cls = entitlements.entitlement_factory(
cfg=cfg, name=options.service, variant=options.variant or ""
)
entitlement = ent_cls(
cfg,
assume_yes=True,
allow_beta=True,
called_name=options.service,
access_only=options.access_only,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should handle the situation where the user provides an invalid service

Copy link
Collaborator Author

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\""
        }

uaclient/api/u/pro/services/enable/v1.py
if isinstance(msg, str)
]

progress.finish()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also update the status cache and send the activity ping to the CS server here ?

Copy link
Collaborator Author

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)

@orndorffgrant
Copy link
Collaborator Author

@lucasmoura updated!

@orndorffgrant
Copy link
Collaborator Author

except wait I broke unit tests... fixing

@orndorffgrant
Copy link
Collaborator Author

@lucasmoura okay now looking good :)

@lucasmoura
Copy link
Contributor

CI error is unrelated to this change

@lucasmoura lucasmoura merged commit a154858 into main Apr 5, 2024
23 of 24 checks passed
@lucasmoura lucasmoura deleted the api-enable-finally branch April 5, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@renanrodrigo renanrodrigo renanrodrigo approved these changes

@lucasmoura lucasmoura lucasmoura approved these changes

Assignees
No one assigned
Labels
next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@orndorffgrant @lucasmoura @renanrodrigo