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

u.pro.services.dependencies.v1 #2961

Merged
merged 5 commits into from
Feb 29, 2024
Merged

u.pro.services.dependencies.v1 #2961

merged 5 commits into from
Feb 29, 2024

Conversation

orndorffgrant
Copy link
Collaborator

@orndorffgrant orndorffgrant commented Feb 23, 2024 •
edited
Loading

Why is this needed?

This is part two of prep for the enable api. This introduces the dependencies endpoint as described in the spec.

Test Steps

New tests should pass

Checklist

Does this PR require extra reviews?

  • Yes
  • No

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 23, 2024 •
edited by orndorffgrant
Loading

🌎 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.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 23, 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: Documentation changes addressed in #2962

👍 this comment to confirm that this is correct.

This was referenced Feb 23, 2024
Merged
Closed
@orndorffgrant orndorffgrant force-pushed the services-dependencies branch 2 times, most recently from df20893 to e8dc51d Compare February 23, 2024 16:59
lucasmoura
lucasmoura approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@lucasmoura lucasmoura left a comment

Choose a reason for hiding this comment

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

Even though we are already testing this on the new integration test, I think we should also add the new endpoint here:
https://github.com/canonical/ubuntu-pro-client/blob/main/features/api.feature#L3

Just for consistency reasons

@orndorffgrant
tests: remove stdlib detail from unit test
21c53ca 
This message is a detail of the stdlib and could change and it doesn't
matter to us. The important thing that this test is testing is that
base.UAEntitlement cannot be instantiated itself and will raise a
TypeError.
@orndorffgrant
refactor: required_services now come with a reason
01d5516
@orndorffgrant
refactor: define name as class property rather than abstract instance…
2739647 
… property
@orndorffgrant
api: u.pro.services.dependencies.v1
0a0a946 
This endpoint will return a full list of all service dependencies,
regardless of the current system state. That means it will always return
the same thing, until new services are added, or if we add/remove
dependencies between services.
@orndorffgrant
update-pos
11764e2
@orndorffgrant
Copy link
Collaborator Author

@lucasmoura great catch! added!

renanrodrigo
renanrodrigo approved these changes Feb 29, 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.

LGTM
I honestly am sad about the EntitlementWithMessage name, but I have no better idea to suggest so

@renanrodrigo renanrodrigo merged commit b35301a into next-v32 Feb 29, 2024
21 of 24 checks passed
@renanrodrigo renanrodrigo deleted the services-dependencies branch February 29, 2024 03:44
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
None yet
Projects
None yet
Milestone
No milestone
Development

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