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

instantiate yumrepo & yum::config directly #148

Merged
merged 1 commit into from
Nov 13, 2019
Merged

instantiate yumrepo & yum::config directly #148

merged 1 commit into from
Nov 13, 2019

Conversation

igalic
Copy link

@igalic igalic commented Nov 12, 2019

Pull Request (PR) description

Since we're not doing any dynamic creation of potentially unknown
resources, we want to use yumrepo and yum::config directly, rather
than going thru Resource

This Pull Request (PR) fixes the following issues

none; only my confusion when reading the code.

Since we're not doing any dynamic creation of potentially unknown
resources, we want to use `yumrepo` and `yum::config` directly, rather
than going thru `Resource`
@@ -133,8 +133,8 @@

$normalized_repos.each |$yumrepo, $attributes| {
if member($_managed_repos_minus_exclusions, $yumrepo) {
Resource['yumrepo'] {
$yumrepo: * => $attributes,
yumrepo { $yumrepo:
Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite identical behaviour. In the original form, I suspect there isn't a duplicate resource situation if the yumrepo has already been declared. On the other hand, I don't think this is actually the case here looking through 1fd551c

Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

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

I think this is much clearer (assuming we haven't overlooked some corner-case reason the original author needed to write it this way).

@alexjfisher
Copy link
Member

@lamawithonel - let us know if we're accidentally breaking something! ;)

@lamawithonel
Copy link

I don't recall why I did that. I worked on a replacement yumrepo resource type for a short time, but ultimately scrapped it. It might have been a mix-up with another version to swap out the type. In any case, you're right, it's unneeded the way it is now.

lgtm 👍

@igalic igalic merged commit db98b7a into voxpupuli:master Nov 13, 2019
@igalic igalic deleted the fix/resource-direct branch November 13, 2019 12:19
@alexjfisher
Copy link
Member

@lamawithonel Many thanks for the feedback.

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.

3 participants