-
Notifications
You must be signed in to change notification settings - Fork 125
Move get_satellite_capsule_repos logic to VersionedContent class #18850
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
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
… test file Co-authored-by: JacobCallahan <[email protected]>
get_satellite_capsule_repos
to VersionedContent.REPOSThere 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.
ACK on the concept of the changes. However, @ogajduse can you verify this fits with your intent? Also, not sure if it's better to reference the settings to get the x_y release or to pull that from a self._satellite
attribute if available.
@copilot pre-commit checks are failing due to trailing whitespace characters. please adjust.
|
Co-authored-by: JacobCallahan <[email protected]>
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.
I'd question the need for having the get_satellite_capsule_repos
function. There may be place for it, but I'd say it could be flattened if we'd use REPOS
and CAPSULE_CDN_REPOS
cached properties defined in the VersionedContent
class.
f'rhel-{os_major_ver}-for-x86_64-baseos-rpms', | ||
f'rhel-{os_major_ver}-for-x86_64-appstream-rpms', |
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.
There already is an existing implementation in
robottelo/robottelo/host_helpers/contenthost_mixins.py
Lines 25 to 26 in adf5ff5
'rhel_bos': constants.REPOSET[f'rhel{self._v_major}_bos'], | |
'rhel_aps': constants.REPOSET[f'rhel{self._v_major}_aps'], |
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.
Good point! The function now reuses the existing RHEL repository implementation by using self.REPOS['rhel_bos']['id']
and self.REPOS['rhel_aps']['id']
instead of manually generating the repository names. This leverages the same well-tested logic that's already in the VersionedContent class. (2646f16)
f'{product}-{x_y_release}-for-rhel-{os_major_ver}-x86_64-rpms', | ||
f'satellite-maintenance-{x_y_release}-for-rhel-{os_major_ver}-x86_64-rpms', |
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 product repositories could be generated by the existing CAPSULE_CDN_REPOS
cached_property. Again, could it be reused here in this function?
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.
Exactly! The function now uses self.CAPSULE_CDN_REPOS.values()
(and self.SATELLITE_CDN_REPOS.values()
) to generate the product repositories when the parameters match the default configuration. This eliminates the duplication and leverages the existing cached property logic. (2646f16)
Co-authored-by: ogajduse <[email protected]>
You're absolutely right! I've refactored the function to use the existing |
""" | ||
# Set defaults | ||
if x_y_release is None: | ||
x_y_release = ".".join(settings.server.version.release.split('.')[0:2]) |
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.
This won't work for stream
. The repository names will include stream
. It has to be the satellite version. Also, def REPOS
provides capsule repo too, which it shouldn't. It's probably good to add logic based on SATELLITE_VERSION
from robottelo/constants/__init__.py
.
This PR moves the
get_satellite_capsule_repos
function fromtests/foreman/maintain/test_advanced.py
to theVersionedContent
class inrobottelo/host_helpers/contenthost_mixins.py
as requested in the issue.Changes Made
1. Added
get_satellite_capsule_repos()
method toVersionedContent
classrobottelo/host_helpers/contenthost_mixins.py
self._v_major
for OS version detection andsettings.server.version.release
for satellite version defaults2. Updated test file usage
tests/foreman/maintain/test_advanced.py
get_satellite_capsule_repos(...)
tosat_maintain.get_satellite_capsule_repos(...)
3. Code cleanup
get_sat_rhel_version
,get_sat_version
)Validation
The changes maintain backward compatibility while properly organizing the code according to the repository's architecture. The method generates the same repository lists as the original function:
The implementation leverages the existing host object structure where
Satellite
andCapsule
instances inherit fromVersionedContent
through theContentHostMixins
chain, making the method naturally available on both host types.Fixes #12492.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.