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

Fixes #30723: Use Puppet 6.6 evaltrace for progress #272

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

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Aug 27, 2020

9fc5fd3 disabled the progress bar
monkey patch on Puppet 6. Puppet 6.6.0 started to mention the progress
when evaluating resources which can be used to update the progress bar.

Replaces #249

@@ -11,7 +11,7 @@ module Kafo
# change more methods like #done_message or #print_error
class ProgressBar
MONITOR_RESOURCE = %r{\w*MONITOR_RESOURCE ([^\]]+\])}
EVALTRACE_START = %r{/(.+\]): Starting to evaluate the resource}
EVALTRACE_START = %r{/(.+\]): Starting to evaluate the resource( \((\d+) of (\d+)\))?}
Copy link
Member

Choose a reason for hiding this comment

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

Back when I wrote the initial version we couldn't use capture groups in regexes because we needed to support Ruby 2.0. Now we can and that makes the code below a lot easier:

regex = %r{/(?<resource>.+\]): Starting to evaluate the resource( \((?<number>\d+) of (?<total>\d+)\))?}
match = regex.match('/Stage[main]/Example/File[/foo/bar]: Starting to evaluate the resource (1 of 2)')
assert match[:resource] == 'Stage[main]/Example/File[/foo/bar]'
assert match[:number] == '1'
assert match[:total] == '2'

legacy_match = regex.match('/Stage[main]/Example/File[/foo/bar]: Starting to evaluate the resource')
assert legacy_match[:resource] == 'Stage[main]/Example/File[/foo/bar]'
assert legacy_match[:number].nil?
assert legacy_match[:total].nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

Switched for each resource to this method. This is much cleaner to read, thanks for pointing it out.

Comment on lines 56 to 58
end

if (known_resource = find_resource(line_start[1]))
Copy link
Member

Choose a reason for hiding this comment

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

With Puppet 6, we no longer hook in internals. This part never runs:

tracked_resources.each { |r| ::Puppet.info "MONITOR_RESOURCE #{r}" }

That means MONITOR_RESOURCE is a Puppet < 6 thing and @resources is never filled. So if you see line_start[4], you know it's Puppet 6.6 and find_known_resource will never work.

This is the part I struggled with.

I think this should be an elsif and set line in the if line_start[4] part.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note how I changed this find_resource and re-factored the method to actually derive the resource. This does still feel nice so that you get the output as:

Notify[test]
Prefetching example resources for example_type 
File[/foo/bar]

Without deriving the resource as we had before I believe we end up with the reporting more like:

/Stage[main]/Example/Notify[test]
Prefetching example resources for example_type
/Stage[main]/Example/File[/foo/bar]

This is more verbose of output, and from a progress bar standpoint, I would think someone would only care to mentally parse, when reading, the way we do it today (e.g. File[/foo/bar]) rather than the full path. I think the other benefit is that it shows the most relevant info on the screen, for example:

Installing             Info: /Package[foreman-dynflow-sidekiq]: Starting  [6%] [.                       ]

Which, if it was the full path, I think would truncate the end off and user would not see the most important part. Granted, most of this information flies off the screen.

Comment on lines 66 to 69
if (known_resource = find_resource(line_end[1]))
@resources.delete(known_resource) # ensure it's only counted once
@lines += 1
end
Copy link
Member

Choose a reason for hiding this comment

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

This feels wrong. It will always find something. I think that means it always increases @lines by 1, introducing an off-by-1 error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why will this always find something? If find_resource returns nil this won't trigger. I've not seen any issues testing this in practice.

9fc5fd3 disabled the progress bar
monkey patch on Puppet 6. Puppet 6.6.0 started to mention the progress
when evaluating resources which can be used to update the progress bar.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants