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.

Sign up for GitHub

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
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 18 additions & 5 deletions lib/puppet/provider/yumrepo/inifile.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

instances = []

virtual_inifile.each_section do |section|
virtual_inifile(resources).each_section do |section|
# Ignore the 'main' section in yum.conf since it's not a repository.
next if section.name == 'main'

Expand All @@ -111,7 +111,7 @@ def self.instances
# @param resources [Array<Puppet::Type::Yumrepo>] Resources to prefetch.
# @return [void]
def self.prefetch(resources)
repos = instances
repos = instances(resources)
resources.each_key do |name|
provider = repos.find { |repo| repo.name == name }
if provider
Expand Down Expand Up @@ -182,14 +182,27 @@ def self.repofiles

# Build a virtual inifile by reading in numerous .repo files into a single
# virtual file to ease manipulation.
# Will skip any inifile that is found to be currently managed as a file resource
# in the catalog.
# @api private
# @return [Puppet::Provider::Yumrepo::IniConfig::File] The virtual inifile representing
# multiple real files.
def self.virtual_inifile
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 •
edited
Loading

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

end

unless @virtual
@virtual = Puppet::Provider::Yumrepo::IniConfig::File.new
repofiles.each do |file|
@virtual.read(file) if Puppet::FileSystem.file?(file)
index_virtual = true
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) }

end
@virtual.read(file) if Puppet::FileSystem.file?(file) && index_virtual
end
end
@virtual
Expand Down