Skip to content

(PUP-10653) Remove win32/dir constant usage#1125

Merged
adrianiurca merged 1 commit intopuppetlabs:mainfrom
GabrielNagy:PUP-10653/remove-win32-dir-dependency
Sep 11, 2020
Merged

(PUP-10653) Remove win32/dir constant usage#1125
adrianiurca merged 1 commit intopuppetlabs:mainfrom
GabrielNagy:PUP-10653/remove-win32-dir-dependency

Conversation

@GabrielNagy
Copy link
Copy Markdown
Contributor

For Puppet 7 we are dropping the win32/dir dependency as we only used constants from it, which we replaced with environment variables (see: puppetlabs/puppet#8314).

This would become breaking when using the stdlib module with Puppet 7, as the win32 dependencies are no longer provided in the puppet gem.

Replace the usage of the Dir::COMMON_APPDATA with the ALLUSERSPROFILE environment variable.

For Puppet 7 we are dropping the `win32/dir` dependency as we only used
constants from it, which we replaced with environment variables (see:
puppetlabs/puppet#8314).

This would become breaking when using the stdlib module with Puppet 7,
as the win32 dependencies are no longer provided in the puppet gem.

Replace the usage of the `Dir::COMMON_APPDATA` with the
`ALLUSERSPROFILE` environment variable.
@GabrielNagy GabrielNagy requested a review from a team as a code owner September 10, 2020 12:48
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 10, 2020

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 5.31%. Comparing base (dd0d71d) to head (2ed74a3).
⚠️ Report is 587 commits behind head on main.

Files with missing lines Patch % Lines
lib/facter/facter_dot_d.rb 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main   #1125      +/-   ##
========================================
+ Coverage   4.39%   5.31%   +0.91%     
========================================
  Files        185     185              
  Lines       5273    5272       -1     
========================================
+ Hits         232     280      +48     
+ Misses      5041    4992      -49     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@joshcooper
Copy link
Copy Markdown
Contributor

joshcooper commented Sep 10, 2020

Since we dropped support for 2003 years ago, then AKAIK Dir::COMMON_APPDATA should always equal to ENV['ALLUSERSPROFILE']? If so, then we should merge this to the earliest supported stdlib version, preferably whatever version is commonly used with puppet5. In other words, it's not actually a breaking change.

If we instead merge to main and release a new major version of stdlib, then existing stdlib versions will fail with puppet7. Users will be forced to update to the latest stdlib in order to deploy puppet7, but that can be difficult when stdlib is a transitive dependency for so many puppet modules, as each modules' metadata needs to be updated to accept the next major. I'd like to avoid that if possible given the change produces the same result, just without the win32-dir gem.

@DavidS
Copy link
Copy Markdown
Contributor

DavidS commented Sep 11, 2020

From a naive reading of the code (I'm still trying to catch up with everything that happened over the last two months) this does not look like a breaking change, especially as you seem to be saying that after Windows Server 2003 this is a noop change anyways? If my understanding is correct, why would it then be a problem merging this a bug fix and asking people to use this as the minimum version to use on puppet 7?

@joshcooper core puppet tools (puppet module install, r10k, the forge, etc) are unable to negotiate installing a non-latest module version matching local puppet version requirements, AND we provide a lot of value through large compatibility ranges for folks still running legacy puppet versions along-side newer installs. Therefore main/the latest released version are "the earliest supported stdlib version" and likely the "version is commonly used with puppet5".

@adrianiurca adrianiurca merged commit e2442f6 into puppetlabs:main Sep 11, 2020
@joshcooper
Copy link
Copy Markdown
Contributor

If my understanding is correct, why would it then be a problem merging this a bug fix

I was mistakenly under the impression that main was for the next major release while 4.x, etc branches were for older stdlib releases. But it looks like those branches don't exist anymore and there's just main, so 👍 for all the reasons you describe. Thanks!

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.

6 participants