Implement stdlib 9 compatibility #657

bastelfreak · 2023-07-06T10:03:56Z

stdlib 9 dropped the is_function_available() function. I reworked the code to check if pe_anchor is defined. The code now works with Puppet core functions and is compatible with Puppet 7/8, probably 6, and does not rely on stdlib.

bastelfreak · 2023-07-06T10:51:12Z

manifests/params.pp

-  # Instead check for the `is_pe` fact or if a PE provided function is available
-  $_is_pe = (getvar('::is_pe') or is_function_available('pe_compiling_server_version'))
-  if $_is_pe {
+  if $facts['is_pe'] {


ah derp, the fact is only available on PE primaries/compilers.

smortex

Some in-line comments. I will push some local changes to your branch to see if it helps.

smortex · 2023-07-26T18:03:23Z

manifests/params.pp

-  $_is_pe = (getvar('::is_pe') or is_function_available('pe_compiling_server_version'))
+  # To determine if this is a PE installation or not, we check if the `pe_anchor` resource type exists
+  # it's provided by the puppet_enterprise module
+  $_is_pe = defined('pe_anchor')


It looks like the test suite rely on the is_pe variable and removing it from the condition break the test suite. This PR being about updating dependencies, I think we can ignore this compatibility thing for now (assuming we trust the comments), and only focus on supporting recent stdlib?

puppetlabs/puppetlabs-stdlib#1386 might be useful to keep the current behavior (see bellow).

smortex · 2023-07-26T18:04:43Z

spec/classes/puppet_agent_spec.rb

@@ -237,7 +237,7 @@ def global_facts(facts, os)
        before(:each) do
          Puppet::Parser::Functions.newfunction(:pe_build_version, type: :rvalue) { |_args| '2000.0.0' }
          Puppet::Parser::Functions.newfunction(:pe_compiling_server_aio_build, type: :rvalue) { |_args| '1.10.100' }
-          Puppet::Parser::Functions.newfunction(:pe_compiling_server_version, type: :rvalue) { |_args| '2.20.200' }
+          Puppet::Parser::Functions.newfunction(:defined, type: :rvalue) { |_args| true }


Mocking defined to return true might break things. According to the tests I did with the module it seems to be fine, but it feel fragile 😨.

I'm not sure I understand why this is getting mocked here; @bastelfreak can you shed some light?

bastelfreak · 2023-07-26T18:49:56Z

manifests/params.pp

@@ -44,7 +44,7 @@
  # and greping for "puppet enterprise". With Puppet 4 and PE 2015.2, there
  # is no longer a "PE Puppet", and so that fact will no longer work.
  # Instead check for the `is_pe` fact or if a PE provided function is available
-  $_is_pe = (getvar('::is_pe') or is_function_available('pe_compiling_server_version'))
+  $_is_pe = (getvar('::is_pe') or stdlib::has_function('pe_compiling_server_version'))


I'm fine with this, but we need to keep in mind that it now requires stdlib 9.3. with my defined() it works on all stdlib versions

Yes, I am not happy with this neither… 😭

I am currently fighting with bolt to use apply with modules that support stdlib ">= 9" but Bolt's apply rely on this module which currently only support stdlib "< 9".

FYI I rolled out my code with defined() at a few customers and that works fine as expected

It is fragile but avoids the need for bumping the minimum version of stdlib. I have no PE anywhere around, so I am not really concerned about this fragility, if it is okay, then it is fine! I'll rework the commits I added to take this into account. If it fix CI and you can test this in the field to validate the change, I think we will be good to go.

I'll rework the commits I added to take this into account. If it fix CI and you can test this in the field to validate the change, I think we will be good to go.

Code pushed. CI failures are related to my commit messages on the commits that need merging, so if you can confirm it fix your issue on PE, you can merge the commits (fixup/squash) and I think CI will be all green 😁

smortex · 2023-08-02T03:03:11Z

I have squashed my changes in your commits, and CI seems happy with it.

@bastelfreak had you the opportunity to check that the behavior was not broken by my change in the real world with PE? Is this PR ready for review 😁 😉 😉?

bastelfreak · 2023-08-02T07:11:34Z

@smortex yes it's working fine :)

smortex

This looks reasonable to me: the minor "issues" are not blockers and given the bad situation of the module (prevent usage of latest stdlib), this is a move in the right direction. As it is now, it is still compatible with previous versions of stdlib so AFAIAC this is 💯!

joshcooper

Thank you @bastelfreak! We're heads down working on upcoming releases and can take a look at this next week. In the meantime could you update the summary to describe why this change is needed? I think I understand, but I admit I haven't been paying complete attention to the strict & stdlib deprecated functions issues.

stdlib 9 dropped the is_function_available() function. I reworked the code to check if `pe_anchor` is defined. The code now works with Puppet core functions and is compatible with Puppet 7/8, probably 6, and does not rely on stdlib.

bastelfreak · 2023-08-09T16:03:07Z

@joshcooper I updated the git commit with the description and added it to the PR title as well.

I want to point out that puppetlabs/stdlib 9 was released almost 2 months ago. PE customers were not able to upgrade because this module isn't compatible. The CAT team worked hard to upgrade all of their modules. I still don't understand why the puppetdb and puppet_agent module are not owned by them / the CAT team isn't listed as code owners. From my point of view (also also from the PE customers I represent) this doesn't make sense. I pointed it out multiple times in the past years and never got a proper explanation for this. Also this wasn't changed. There was zero activity from the current owners about making puppet_agent compatible with stdlib 9. It took quite a long time to get a response here. I fail to understand why some modules are not maintained at all / maintained by other teams instead of the module with the main focus on maintaining modules. I would love to see a migration to the CAT team to ship enhancements faster to customers and to have a single source of configuration/guidelines/team for modules.

joshcooper · 2023-08-18T21:18:37Z

Hi @bastelfreak, I understand your frustration, the current approach is not working for anyone. I've been advocating for moving these and *_core modules to the ~~Modules~~, ~~IAC~~ and CAT teams for literally years. But as usual, there are too many things to do and not enough people. I am hopeful this will change in the near future though. I'll have a chat with folks on my end.

bastelfreak · 2023-08-23T19:04:00Z

@tvpartytonight thanks for merging <3

…ble fix puppetlabs/puppetlabs-puppet_agent#657

bastelfreak force-pushed the stdlib9 branch from d49cd91 to a5f9838 Compare July 6, 2023 10:39

bastelfreak commented Jul 6, 2023

View reviewed changes

bastelfreak force-pushed the stdlib9 branch from a5f9838 to 3bb9761 Compare July 10, 2023 09:37

bastelfreak added 2 commits July 10, 2023 11:40

(maint) .fixtures.yml: delete legacy symlink

ef49601

(maint) Add stdlib9 support; test against branch

6538137

bastelfreak force-pushed the stdlib9 branch 3 times, most recently from 7d73255 to e969c7c Compare July 10, 2023 10:42

smortex reviewed Jul 26, 2023

View reviewed changes

bastelfreak commented Jul 26, 2023

View reviewed changes

smortex force-pushed the stdlib9 branch 2 times, most recently from 18b09d1 to 669bfc0 Compare August 2, 2023 02:39

bastelfreak marked this pull request as ready for review August 2, 2023 07:11

bastelfreak requested a review from a team as a code owner August 2, 2023 07:11

smortex approved these changes Aug 2, 2023

View reviewed changes

joshcooper reviewed Aug 9, 2023

View reviewed changes

bastelfreak added 2 commits August 9, 2023 17:54

(maint) Drop unused master_agent_version variable

ac50f84

bastelfreak force-pushed the stdlib9 branch from 669bfc0 to ac50f84 Compare August 9, 2023 15:56

kenyon mentioned this pull request Aug 23, 2023

replace is_function_available with stdlib::has_function #662

Closed

tvpartytonight approved these changes Aug 23, 2023

View reviewed changes

tvpartytonight merged commit f9554d9 into puppetlabs:main Aug 23, 2023
16 checks passed

bastelfreak deleted the stdlib9 branch August 23, 2023 19:03

kenyon added a commit to kenyon/puppet that referenced this pull request Sep 3, 2023

Puppetfile: puppet_agent: use commit that contains is_function_availa…

debcf70

…ble fix puppetlabs/puppetlabs-puppet_agent#657

AriaXLi added the enhancement New feature or request label Sep 20, 2023

kenyon mentioned this pull request Nov 22, 2023

.fixtures.yml: test with the main branches of modules #683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement stdlib 9 compatibility #657

Implement stdlib 9 compatibility #657

bastelfreak commented Jul 6, 2023 •

edited

Loading

bastelfreak Jul 6, 2023

smortex left a comment

smortex Jul 26, 2023

smortex Jul 26, 2023

tvpartytonight Aug 18, 2023

bastelfreak Jul 26, 2023

smortex Jul 26, 2023

bastelfreak Jul 26, 2023

smortex Jul 26, 2023

smortex Jul 26, 2023

smortex commented Aug 2, 2023

bastelfreak commented Aug 2, 2023

smortex left a comment

joshcooper left a comment

bastelfreak commented Aug 9, 2023

joshcooper commented Aug 18, 2023

bastelfreak commented Aug 23, 2023

Implement stdlib 9 compatibility #657

Implement stdlib 9 compatibility #657

Conversation

bastelfreak commented Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

smortex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smortex commented Aug 2, 2023

bastelfreak commented Aug 2, 2023

smortex left a comment

Choose a reason for hiding this comment

joshcooper left a comment

Choose a reason for hiding this comment

bastelfreak commented Aug 9, 2023

joshcooper commented Aug 18, 2023

bastelfreak commented Aug 23, 2023

bastelfreak commented Jul 6, 2023 •

edited

Loading