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

Use session-scoped inventory by default #55

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion manifester/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ def inventory(details, sync, offline_token):
border = "-" * 38
if sync:
helpers.update_inventory(
Manifester(minimal_init=True, offline_token=offline_token).subscription_allocations
Manifester(minimal_init=True, offline_token=offline_token).subscription_allocations,
sync=True,
)
inv = helpers.load_inventory_file(Path(settings.inventory_path))
if not details:
Expand Down
42 changes: 35 additions & 7 deletions manifester/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,15 +171,43 @@ def load_inventory_file(file):
return yaml.load(f, Loader=yaml.FullLoader) or []


def update_inventory(inventory_data):
def _dump_inventory_file(inventory_path, inventory):
"""Write inventory data to local inventory file."""
with inventory_path.open("w") as inventory_file:
yaml.dump(inventory, inventory_file, allow_unicode=True)


def _update_inventory(inventory_path, inventory, allocation):
"""Add new allocation, remove and recreate inventory file."""
inventory.append(allocation)
inventory_path.unlink()
inventory_path.touch()
_dump_inventory_file(inventory_path, inventory)
Comment on lines +174 to +185
Copy link
Member

Choose a reason for hiding this comment

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

could this be simplified?

Suggested change
def _dump_inventory_file(inventory_path, inventory):
"""Write inventory data to local inventory file."""
with inventory_path.open("w") as inventory_file:
yaml.dump(inventory, inventory_file, allow_unicode=True)
def _update_inventory(inventory_path, inventory, allocation):
"""Add new allocation, remove and recreate inventory file."""
inventory.append(allocation)
inventory_path.unlink()
inventory_path.touch()
_dump_inventory_file(inventory_path, inventory)
def _update_inventory(inventory_path, inventory, allocation):
"""Add new allocation, remove and recreate inventory file."""
inventory.append(allocation)
inventory_path.unlink()
inventory_path.touch()
yaml.dump(inventory, inventory_path, allow_unicode=True)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@JacobCallahan Per the PyYAML docs, yaml.dump needs an open file in order to write to it directly. Your proposed changed throws an AttributeError: 'PosixPath' object has no attribute 'write'. Did you mean: 'drive'? when it tries to dump to the Path object. So we need a second line before yaml.dump to open the inventory file. I think it's slightly cleaner to move that action into a separate helper function since it gets used in three places, but that part isn't strictly necessary.



def update_inventory(inventory_data, sync=False, remove=False, uuid=None):
"""Replace the existing inventory file with current subscription allocations."""
inventory_path = Path(settings.inventory_path)
if load_inventory_file(inventory_path):
inventory_path.unlink()
inventory_path.touch()
if inventory_data != []:
with inventory_path.open("w") as inventory_file:
yaml.dump(inventory_data, inventory_file, allow_unicode=True)
if sync:
if load_inventory_file(inventory_path):
inventory_path.unlink()
inventory_path.touch()
if inventory_data != []:
_dump_inventory_file(inventory_path, inventory_data)
elif remove:
inv = load_inventory_file(inventory_path)
_dump_inventory_file(inventory_path, [alloc for alloc in inv if uuid not in alloc["uuid"]])
Copy link
Preview

Copilot AI Nov 22, 2024

Choose a reason for hiding this comment

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

The condition 'uuid not in alloc["uuid"]' should be 'uuid != alloc["uuid"]' to correctly filter out the allocation with the matching UUID.

Suggested change
_dump_inventory_file(inventory_path, [alloc for alloc in inv if uuid not in alloc["uuid"]])
_dump_inventory_file(inventory_path, [alloc for alloc in inv if uuid != alloc["uuid"]])

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
else:
current_allocation = next(
iter([alloc for alloc in inventory_data if alloc["uuid"] == uuid])
)
inv = load_inventory_file(inventory_path)
uuids = [alloc["uuid"] for alloc in inv]
if current_allocation["uuid"] not in uuids:
_update_inventory(inventory_path, inv, current_allocation)
else:
inv = [alloc for alloc in inv if uuid not in alloc["uuid"]]
_update_inventory(inventory_path, inv, current_allocation)


def fake_http_response_code(good_codes=None, bad_codes=None, fail_rate=0):
Expand Down
24 changes: 11 additions & 13 deletions manifester/manifester.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,9 @@ def create_subscription_allocation(self):
cmd_kwargs=allocation_data,
).json()
logger.debug(f"Received response {self.allocation} when attempting to create allocation.")
self.allocation_uuid = self.allocation["body"]["uuid"]
self.allocation_uuid = (
self.allocation.uuid if self.is_mock else self.allocation["body"]["uuid"]
)
if self.simple_content_access == "disabled":
simple_retry(
self.requester.put,
Expand All @@ -185,7 +187,7 @@ def create_subscription_allocation(self):
f"Subscription allocation created with name {self.allocation_name} "
f"and UUID {self.allocation_uuid}"
)
update_inventory(self.subscription_allocations)
update_inventory(self.subscription_allocations, uuid=self.allocation_uuid)
return self.allocation_uuid

def delete_subscription_allocation(self, uuid=None):
Expand All @@ -196,14 +198,14 @@ def delete_subscription_allocation(self, uuid=None):
"proxies": self.manifest_data.get("proxies"),
"params": {"force": "true"},
}
if self.is_mock:
self.allocation_uuid = self.allocation_uuid.uuid
response = simple_retry(
self.requester.delete,
cmd_args=[f"{self.allocations_url}/{uuid if uuid else self.allocation_uuid}"],
cmd_kwargs=data,
)
update_inventory(self.subscription_allocations)
update_inventory(
self.subscription_allocations, remove=True, uuid=uuid if uuid else self.allocation_uuid
)
return response

def add_entitlements_to_allocation(self, pool_id, entitlement_quantity):
Expand Down Expand Up @@ -322,15 +324,15 @@ def process_subscription_pools(self, subscription_pools, subscription_data):
f"{subscription_data['name']} to the allocation."
)
self._active_pools.append(match)
update_inventory(self.subscription_allocations)
update_inventory(self.subscription_allocations, uuid=self.allocation_uuid)
break
elif add_entitlements.status_code == SUCCESS_CODE:
logger.debug(
f"Successfully added {subscription_data['quantity']} entitlements of "
f"{subscription_data['name']} to the allocation."
)
self._active_pools.append(match)
update_inventory(self.subscription_allocations)
update_inventory(self.subscription_allocations, uuid=self.allocation_uuid)
break
else:
raise RuntimeError(
Expand Down Expand Up @@ -405,11 +407,8 @@ def trigger_manifest_export(self):
local_file.write_bytes(manifest.content)
manifest.path = local_file
manifest.name = self.manifest_name
if self.is_mock:
manifest.uuid = self.allocation_uuid.uuid
else:
manifest.uuid = self.allocation_uuid
update_inventory(self.subscription_allocations)
manifest.uuid = self.allocation_uuid
update_inventory(self.subscription_allocations, uuid=self.allocation_uuid)
return manifest

def get_manifest(self):
Expand Down Expand Up @@ -437,4 +436,3 @@ def __enter__(self):
def __exit__(self, *tb_args):
"""Deletes subscription allocation on teardown unless using CLI."""
self.delete_subscription_allocation()
update_inventory(self.subscription_allocations)
25 changes: 18 additions & 7 deletions tests/test_manifester.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,13 @@
+ "".join(random.choices(string.ascii_letters, k=8)),
"type": "Satellite",
"version": f"{MANIFEST_DATA['sat_version']}",
"entitlementQuantity": sum(d["quantity"] for d in MANIFEST_DATA["subscription_data"]),
"url": f"{MANIFEST_DATA['url']['allocations']}/{SUB_ALLOCATION_UUID}",
"entitlementsAttachedQuantity": sum(
d["quantity"] for d in MANIFEST_DATA["subscription_data"]
),
"simpleContentAccess": f"{MANIFEST_DATA['simple_content_access']}",
}
]
],
"status_code": 200,
}


Expand Down Expand Up @@ -148,9 +150,9 @@ def get(self, *args, **kwargs):
self.pool_response["body"] += SUB_POOL_RESPONSE["body"]
return self
if args[0].endswith("allocations") and self._has_offset:
if kwargs["params"]["offset"] != 50:
if kwargs["params"]["offset"] != 100:
self.allocations_response = {"body": []}
for _x in range(50):
for _x in range(100):
self.allocations_response["body"].append(
{
"uuid": f"{uuid.uuid4().hex}",
Expand Down Expand Up @@ -197,6 +199,15 @@ def delete(self, *args, **kwargs):
self._good_codes = [204]
return self

def __repr__(self):
"""Return a string representation of the RhsmApiStub instance."""
inner = ", ".join(
f"{k}={v}"
for k, v in self.__dict__.items()
if not k.startswith("_") and not callable(v)
)
return f"{self.__class__.__name__}({inner})"


def test_basic_init():
"""Test that manifester can initialize with the minimum required arguments."""
Expand All @@ -211,7 +222,7 @@ def test_create_allocation():
"""Test that manifester's create_subscription_allocation method returns a UUID."""
manifester = Manifester(manifest_category=MANIFEST_DATA, requester=RhsmApiStub(in_dict=None))
allocation_uuid = manifester.create_subscription_allocation()
assert allocation_uuid.uuid == SUB_ALLOCATION_UUID
assert allocation_uuid == SUB_ALLOCATION_UUID


def test_negative_simple_retry_timeout():
Expand Down Expand Up @@ -300,7 +311,7 @@ def test_invalid_sat_version():
def test_update_inventory():
"""Test that inventory file is populated with expected contents after updating."""
manifester = Manifester(manifest_category=MANIFEST_DATA, requester=RhsmApiStub(in_dict=None))
update_inventory(manifester.subscription_allocations)
update_inventory(manifester.subscription_allocations, sync=True, uuid=SUB_ALLOCATION_UUID)
assert (
load_inventory_file(Path(MANIFEST_DATA["inventory_path"]))
== SUB_ALLOCATIONS_RESPONSE["body"]
Expand Down
Loading