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

(PA-5807) Ignore yumrepo files managed by Puppet file resource #72

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cthorn42
Copy link
Collaborator

Previously we attempted to manage every repo that was found in the yumrepos directory. If one of those repos was a Puppet File resource, yumrepo would manage the yumrepo the Puppet File resource was creating. This casued some idepontecy issues.
This change will make it so yumrepo will not attempt to manage any file resource it finds that is found in the yumrepo directory.

Previously we attempted to manage every repo that was found in the yumrepos
directory. If one of those repos was a Puppet File resource, yumrepo
would manage the yumrepo the Puppet File resource was creating. This casued
some idepontecy issues.
This change will make it so yumrepo will not attempt to manage any file resource
it finds that is found in the yumrepo directory.
@cthorn42 cthorn42 requested a review from a team as a code owner July 23, 2024 15:26
@@ -82,10 +82,10 @@ def new_physical_file(file)
# @api public
# @return [Array<Puppet::Provider>] providers generated from existing yum
# repository definitions.
def self.instances
def self.instances(resources = nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since instances is a public provider method that overrides https://github.com/puppetlabs/puppet/blob/82ad86ea21ecf20a956db6bf46140940caccd0cc/lib/puppet/provider.rb#L382 it's surprising for this method to have a different signature than its parent. Since the instances method may be called in two different contexts (during prefetch or when running puppet resource yumrepo) I'd recommend creating a private class method containing all of the old instances code and then call it with different arguments from instances and prefetch, something like:

Suggested change
def self.instances(resources = nil)
def self.instances
instances_with_resources
end
def self.instances_with_resources(resources = nil)
# existing instances code
end
private_class_method :instances_with_resources
def self.prefetch(resources)
repos = instances_with_resources(resources)
...
end

def self.virtual_inifile(resources = nil)
current_yumrepo_inis = []
resources&.each do |key|
current_yumrepo_inis << key[1]
Copy link
Contributor

@joshcooper joshcooper Jul 24, 2024

Choose a reason for hiding this comment

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

Since resources is a Hash, you could extract the values directly:

-    current_yumrepo_inis = []
-    resources&.each do |key|
-      current_yumrepo_inis << key[1]
+    current_yumrepo_inis = resources&.values

Also to avoid resources defaulting to nil while current_yumrepo_inis defaults to [], I'd default the former to:

def self.instances_with_resources(resources = [])

Then you can drop all of the safe nav operators

unless current_yumrepo_inis.empty?
current_yumrepo_inis.each do |index|
index_virtual = false if index.catalog.resource('File', file)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

We can stop enumerating over current_yumrepo_inis as soon as we find the first File resource with path file. A common idiom to short-circuit is to do:

-          current_yumrepo_inis.each do |index|
-            index_virtual = false if index.catalog.resource('File', file)
-          end
+          index_virtual = !current_yumrepo_inis.any? { |index| index.catalog.resource('File', file) }

@joshcooper
Copy link
Contributor

Could you add a test to show if a file is managed by both a file and yumrepo, then the latter doesn't purge it? I think this has to be done as a beaker test given the way the rspec tests are currently structured.

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