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

Copy the mtime on puppet generate types #9206

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

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented Jan 10, 2024

When installing a new module it can maintain the file timestamps from the original tarball (like g10 does). puppet generate types uses the timestamp of when it ran.

If you update a module to a release that was created before it ran puppet types generate, it will never update the types. This can happen with a module downgrade, or just when you regenerated types manually. By using --force the timestamps are ignored, but that's only a workaround.

This new approach copies the mtime from the source file to the generated cache file. It is only considered up to date if the mtime matches.

Reviewer notes:

  • I haven't had a chance to test this yet, so take that into account
  • It does a stat twice now - this could be cached
  • It's relying on the implicit mtime comparison (using <=> on File::Stat) while in copying it is explicit - should this be changed in comparison?

@ekohl ekohl requested a review from a team as a code owner January 10, 2024 11:47
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@ekohl
Copy link
Contributor Author

ekohl commented Jan 12, 2024

The test failure is in related code, but the error message lacks detail.

When installing a new module it can maintain the file timestamps from
the original tarball (like g10 does). puppet generate types uses the
timestamp of when it ran.

If you update a module to a release that was created before it ran
puppet types generate, it will never update the types. This can happen
with a module downgrade, or just when you regenerated types manually. By
using --force the timestamps are ignored, but that's only a workaround.

This new approach copies the mtime from the source file to the generated
cache file. It is only considered up to date if the mtime matches.
@ekohl ekohl force-pushed the copy-mtime-for-generate-types branch from eed5a85 to c0a5fca Compare January 12, 2024 10:21
@joshcooper
Copy link
Contributor

Thanks @ekohl! I've been bitten by this issue too when downgrading a module. The test is failing because it assumes puppet generate types --force will always update the mtime of the generated files

Of course, that's no longer true, since the generated file's mtime is set to whatever the input file's mtime was. I think the test would need to be modified to verify the mtimes are the same, but the File::Stat('..').ino are different (and maybe skip the test on Windows)?

Also @jonathannewman and @justinstoller would this introduce/cause problems for code manager?

@joshcooper joshcooper added the bug Something isn't working label Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants