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

Add subscription_allocations property #25

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

synkd
Copy link
Collaborator

@synkd synkd commented Jan 19, 2024

As part of the Manifester inventory management effort, this PR adds a subscription_allocations property (filtered by the username_prefix setting) to the Manifester object. To enable this change, it abstracts the paginated API data processing functionaliy from the subscription_pools property to a helper function capable of handling GET requests to either the allocations or pools endpoints.

Additionally, this PR adds a new unit test and mock endpoint to the RhsmApiStub class's get() method.

As part of the Manifester inventory management effort, this PR adds a
`subscription_allocations` property (filtered by the `username_prefix`
setting)  to the `Manifester` object. To enable this change, it abstracts
the paginated API data processing functionaliy from the `subscription_pools`
property to a helper function capable of handling GET requests to either the
`allocations` or `pools` endpoints.

Additionally, this PR adds a new unit test and mock endpoint to the
RhsmApiStub class's `get()` method.
Copy link

@Griffin-Sullivan Griffin-Sullivan left a comment

Choose a reason for hiding this comment

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

LGTM. There's some redundancy within fetch_paginated_data since you make the API call once and then the same steps are in the while loop. For now I think it's okay, but there's probably a better way to do this so you don't repeat the same logic.

manifest_category=manifest_data, requester=RhsmApiStub(in_dict=None, has_offset=True)
)
for allocation in manifester.subscription_allocations:
assert allocation["name"].startswith(manifest_data["username_prefix"])

Choose a reason for hiding this comment

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

Is there a length of allocations we should also assert for to make sure it's returning the total you expect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so. One of the end goals of this effort is to provide users with a mechanism to clean up their existing subscription allocations, particularly ones of which they are unaware (e.g. allocations that were not cleaned up properly due to test interruption). So having an expectation on the number of allocations returned would be at cross purposes to that goal, in my thinking.

@synkd
Copy link
Collaborator Author

synkd commented Jan 23, 2024

@Griffin-Sullivan If I understand you correctly regarding the redundancy in fetch_paginated_data, then that is intentional. The _offset variable is there because the API endpoints that the function targets return a limit of 50 results per request. However, those endpoints have an offset parameter to allow fetching data beyond the first 50 results. So, if a result returns exactly 50 results, the function increments the '_offsetvariable by 50, then passes that new value as theoffset` parameter in the next API call. This is repeated until it gets a response containing less than 50 results, which should mean it has pulled all available data from that endpoint. Please let me know if I've misunderstood your comment about redundancy.

@synkd synkd merged commit 975da9c into SatelliteQE:master Jan 23, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants