-
Notifications
You must be signed in to change notification settings - Fork 20
refactor: Use __ad_integration_rh_distros when checking distribution
#160
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
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces hardcoded Red Hat clones list with the dynamic Class diagram for distribution check logic updateclassDiagram
class MainTask {
+ansible_distribution: string
+ansible_distribution_version: string
+ad_integration_manage_crypto_policies: bool
+ad_integration_allow_rc4_crypto: bool
+crypto_policies_policy: string
}
class __lsr_redhat_clones {
+["CentOS", "RedHat", "AlmaLinux", "RockyLinux", ...]
}
MainTask --> __lsr_redhat_clones: uses for distribution checks
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Verify that __lsr_redhat_clones is defined in your role defaults and includes all intended distros (Alma, Rocky, etc.) before using it in the when statements.
- Consider using ansible_distribution_major_version for the version checks to simplify the when conditions and make them more readable.
- The two crypto‐policy when blocks share very similar logic—extracting the distribution check into a separate variable or combined conditional could reduce duplication and improve maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Verify that __lsr_redhat_clones is defined in your role defaults and includes all intended distros (Alma, Rocky, etc.) before using it in the when statements.
- Consider using ansible_distribution_major_version for the version checks to simplify the when conditions and make them more readable.
- The two crypto‐policy when blocks share very similar logic—extracting the distribution check into a separate variable or combined conditional could reduce duplication and improve maintainability.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Fixes linux-system-roles#39 Signed-off-by: Hayley Hughes <[email protected]>
|
Apologies for the force push, I forgot to signoff the commit |
richm
left a comment
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.
Use __ad_integration_rh_distros instead of __lsr_redhat_clones, or __ad_integration_rh_distros_fedora if this applies to Red Hat clones and Fedora
Signed-off-by: Hayley Hughes <[email protected]>
|
Done 🙂 I assume this check can't be simplified using Lines 113 to 115 in bbc8ef7
|
__lsr_redhat_clones when checking distribution__ad_integration_rh_distros when checking distribution
|
[citest] |
Enhancement:
Use
__ad_integration_rh_distrosinstead of a fixed list when checking the distro release for determining the crypto policyReason:
Adds support for Alma and Rocky Linux
Fixes #39
Summary by Sourcery
Use a dynamic clone list to determine supported RHEL-based distributions when checking crypto policies to add support for Alma and Rocky Linux.
New Features:
Enhancements: