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

Jump to bottom

(PUP-11597) Generate types when any module libs are updated #8928

Open
wants to merge 1 commit into
base: 7.x
Choose a base branch
from
Open

(PUP-11597) Generate types when any module libs are updated #8928

wants to merge 1 commit into from

Conversation

natemccurdy
Copy link
Contributor

@natemccurdy natemccurdy commented Jul 26, 2022 •
edited
Loading

Fixes PUP-11597

Before

Prior to this, the puppet generate types command would only detect resource type changes if the type file itself changed.

For example, if your type was defined at <module>/lib/puppet/type/foo.rb, a change in it would be detected.

But if your type includes libraries from separate Ruby files in lib/puppet/* or even in the PuppetX namespace like lib/puppet_x/*, changes to those other files would not be detected.

The consequence of that behavior is that updates to the signature of a type that come from "other" ruby files would not trigger a metadata re-creation and those types would suffer from the environment-isolation problem (SERVER-94) that puppet generate types was meant to solve.

After

This fixes that problem by updating puppet generate types to scan for changed Ruby files within a module's lib/* directory.

It uses the same Puppet::FileSystem::stat operation to compare mtime of the Ruby files in lib/* relative to the generated metadata file. And if the metadata is older than any of the Ruby files in lib/*, the metadata is regenerated.

The one slight-downside to my solution is that generation of type metadata will possibly happen more often than before and possibly more often than is needed. However, generating types is fast, and it's usually only done when code updates happen (e.g. via r10k or Code Manager), so the total added load is very close to, if not, negligible.

@natemccurdy natemccurdy requested a review from a team as a code owner July 26, 2022 20:39
@natemccurdy natemccurdy requested a review from a team July 26, 2022 20:39
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@natemccurdy natemccurdy changed the title (PUP-11597) A working, but very likely not ideal fix for PUP-11597 (WIP) (PUP-11597) A working, but very likely not ideal fix for PUP-11597 Jul 26, 2022
@natemccurdy natemccurdy force-pushed the PUP-11597/generate_types_puppetx branch from f9be67a to ac6d139 Compare July 26, 2022 21:47
@natemccurdy natemccurdy changed the title (WIP) (PUP-11597) A working, but very likely not ideal fix for PUP-11597 (WIP) (PUP-11597) Generate types when any module libs are updated Jul 26, 2022
@natemccurdy natemccurdy force-pushed the PUP-11597/generate_types_puppetx branch 2 times, most recently from 10043c0 to 573fbac Compare July 26, 2022 23:10
@natemccurdy
Copy link
Contributor Author

I'm not sure why the spec tests are failing in jruby and windows. Anyone have an idea why?

@natemccurdy natemccurdy changed the title (WIP) (PUP-11597) Generate types when any module libs are updated (PUP-11597) Generate types when any module libs are updated Aug 16, 2022
@joshcooper
Copy link
Contributor

Since CI failed more than a month ago, I have to close and reopen to rekick the GH actions.

@joshcooper joshcooper closed this Aug 30, 2022
@joshcooper joshcooper reopened this Aug 30, 2022
@joshcooper
Copy link
Contributor

@natemccurdy I think Windows tests are failing because of the way the test stubs filesystem access. Perhaps Windows mtime resolution isn't as low as Posix or the time is being truncated?

  1) Puppet::Face[:generate, 0.1.0] when used from an interactive terminal in an environment with two modules containing resource types overwrites if files exists that are not up to date while keeping up to date files
     Failure/Error: expect(stats_before[0] <=> stats_after[0]).to eq(-1)
       expected: -1
            got: 0
       (compared using ==)
```.

joshcooper
joshcooper requested changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@joshcooper joshcooper left a comment

Choose a reason for hiding this comment

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

Just noting that this needs work

@natemccurdy natemccurdy force-pushed the PUP-11597/generate_types_puppetx branch from 573fbac to 8559e28 Compare September 13, 2022 18:04
@natemccurdy natemccurdy force-pushed the PUP-11597/generate_types_puppetx branch 2 times, most recently from 822d21b to 146ca76 Compare January 13, 2023 23:00
@natemccurdy
Copy link
Contributor Author

natemccurdy commented Jan 13, 2023 •
edited
Loading

I wasn't able to figure out a way to get the tests to pass on Windows by faking a passage of time, so I resorted to a sleep(1) which is what the existing tests do.

I don't like that sleep, but the tests pass 🤷 .

@natemccurdy
Copy link
Contributor Author

@joshcooper Is there anything specific here that still needs work?

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@natemccurdy natemccurdy requested a review from a team as a code owner May 2, 2023 16:40
@natemccurdy natemccurdy force-pushed the PUP-11597/generate_types_puppetx branch from 10a5790 to 048af2e Compare May 2, 2023 16:43
@natemccurdy natemccurdy changed the base branch from 6.x to 7.x May 2, 2023 16:43
@joshcooper
Copy link
Contributor

Retriggering tests

@joshcooper joshcooper closed this Oct 25, 2023
@joshcooper joshcooper reopened this Oct 25, 2023
joshcooper
joshcooper reviewed Nov 15, 2023
View reviewed changes
spec/unit/face/generate_spec.rb
genface.types
stats_before = [Puppet::FileSystem.stat(File.join(outputdir, 'test1.pp')), Puppet::FileSystem.stat(File.join(outputdir, 'test2.pp'))]
# Sorry. The sleep is needed because Puppet::FileSystem.touch(<path>, :mtime => Time.now 1000) doesn't work on Windows.
sleep(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

That is surprising because our filesystem code calls FileUtils.touch which eventually passes the mtime through to https://github.com/ruby/ruby/blob/536649f819ed8f2bb0f8f44b1a0ca5c6d1753b24/win32/win32.c#L7692 where SetFileTime is documented in https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-setfiletime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a build just now that uses:

Puppet::FileSystem.touch(puppet_x_lib, :mtime => Time.now   1000)

It failed in the jruby spec tests: https://github.com/puppetlabs/puppet/actions/runs/6882098596/job/18719877970

From what I remember when I first made this PR, the sleep(1) was the only way to get the tests to pass.
Though now that I look at it, the comment about "doesn't work on Windows" might be wrong... it's actually failing on Jruby and the Windows tests are being skipped because of that.

joshcooper
joshcooper reviewed Nov 15, 2023
View reviewed changes
lib/puppet/generate/type.rb Outdated
end
# Check for updates to any module lib files.
module_lib_files = Dir.glob(File.join(@base, "lib", "**", "*.rb"))
module_lib_files.each do |lib|
Copy link
Contributor

@joshcooper joshcooper Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Wondering if it'd be better to use the block form and break as soon as we detect a newer file?

-         module_lib_files = Dir.glob(File.join(@base, "lib", "**", "*.rb"))
-         module_lib_files.each do |lib|
          Dir.glob(File.join(@base, "lib", "**", "*.rb")) do |lib|

Also I'd want to understand how this would affect code deployment times for a "typical" environment with hundreds of modules.

@joshcooper joshcooper added the enhancement New feature or request label Nov 15, 2023
@natemccurdy natemccurdy force-pushed the PUP-11597/generate_types_puppetx branch 2 times, most recently from 13f302a to 37c4da9 Compare November 15, 2023 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshcooper joshcooper joshcooper requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@natemccurdy @puppetlabs-jenkins @joshcooper @CLAassistant