(PUP-11693) Global OptionParser ignores partially matched invalid params

[AriaXLi]

This commit modifies how partial matches are handled when parsing global command line options. Before, Puppet would accept and do nothing with options global options that use '-' instead of '_'. For example, it would accept '--show-diff' and do nothing when the correct name is '--show_diff'.

With this change, while handling any global options given in the command line options, if the command line option does not match existing global options, it will convert '-' to '' and vice versa. Then, check if that matches a valid global option. Additionally, if the command line option needs to convert '-' to '' or vice versa, Puppet will also warn to let the user know a partial match was detected and this behavior will be deprecated in Puppet 9.

[AriaXLi]

Still need to add the warning when a partial match is detected, I forgot to do that today....it'll be a tomorrow problem

[AriaXLi]

Puppet accepts partial matches after my changes:

❯ bundle exec puppet apply --show_diff -e 'file { "/tmp/foo": content => "${facts[system_uptime][seconds]}\n" }'

Notice: Compiled catalog for ariali-c02gm0almd6v.local in environment production in 0.03 seconds
Notice: /Stage[main]/Main/File[/tmp/foo]/content:
--- /tmp/foo	2024-04-11 23:26:49
+++ /var/folders/8m/nrnj3z6j01z51syp94jpjp_m0000gp/T/puppet-file20240411-79648-abs3z7	2024-04-11 23:43:34
@@ -1 +1 @@
-623935
+624940

Notice: /Stage[main]/Main/File[/tmp/foo]/content: content changed '{sha256}1be3421d7961f745b35ee610f16e8bbdcd0c51b52a8fcbbfb5c600b497afe65c' to '{sha256}00005345b341b2f1612a353bc5417ac38ee5b2a82e58ee0c038cd8368b4e5e8b'
Notice: Applied catalog in 0.07 seconds

❯ sleep 1 ; bundle exec puppet apply --show-diff -e 'file { "/tmp/foo": content => "${facts[system_uptime][seconds]}\n" }'

Warning: Partial argument match detected: --show-diff. Partial argument matching will be deprecated in Puppet 9.
Notice: Compiled catalog for ariali-c02gm0almd6v.local in environment production in 0.05 seconds
Notice: /Stage[main]/Main/File[/tmp/foo]/content:
--- /tmp/foo	2024-04-11 23:43:34
+++ /var/folders/8m/nrnj3z6j01z51syp94jpjp_m0000gp/T/puppet-file20240411-79889-5sg0f5	2024-04-11 23:43:53
@@ -1 +1 @@
-624940
+624960

Notice: /Stage[main]/Main/File[/tmp/foo]/content: content changed '{sha256}00005345b341b2f1612a353bc5417ac38ee5b2a82e58ee0c038cd8368b4e5e8b' to '{sha256}785b70f3c57ca3e2400691a14aad3180d7864ac8d3629e742d36a736c16543af'
Notice: Applied catalog in 0.06 seconds

Before my changes it did not:

❯ sleep 1 ; puppet apply --show-diff -e 'file { "/tmp/foo": content => "${facts[system_uptime][seconds]}\n" }'

Notice: Compiled catalog for vm3.oxidized.org in environment production in 0.01 seconds

Notice: /Stage[main]/Main/File[/tmp/foo]/content: content changed '{sha256}9bd523e3860777cd1c2d38207b95a7bc0bd8ed1d47afc839da1378ce71ad28f3' to '{sha256}3b9cd754c5b3f7219943c3ebdf2b51b972980cfc0d478e645784ae2b78699314'

Notice: Applied catalog in 0.01 seconds

[tvpartytonight]

The only thing that I think might be a nice-to-have-but-not-necessary is some text to tell you which option passed in is the partial match. If you think that is a good idea you should go for it, otherwise I think you should just merge it @AriaXLi !

[joshcooper]

It's good to tell users what they should do to remove the warning. Also recommend using the depreciation_warning method. Maybe something like

Suggested change

	Puppet.warning("Partial argument match detected. Partial argument matching will be deprecated in Puppet 9.")
	Puppet.deprecation_warning("Partial argument match detected. The correct argument is '#{...}'. Partial argument matching behavior is deprecated and will be removed in a future release.")

[joshcooper]

Sorry, I left a comment, but it was marked as "pending" so maybe it wasn't visible? In any case, I think there are a couple adjustments needed:

• The warning is triggered anytime an application-specific option is given:

  $ bundle exec puppet apply --execute "notice(0)"
  Warning: Partial argument match detected: --execute. Partial argument matching will be deprecated in Puppet 9.
  

  I think we only want to trigger the warning if an argument is given that doesn't match a global option, but it does match one of the translated versions. So --execute is not a global option (it's specific to the apply application) and shouldn't trigger the warning. The argument --show-diff is also not a global option, but it does match the translated version --show_diff. So in that case we do want to print the warning.

• If the warning is triggered, it would be good to specify what option the user should be using, so they can self-correct. For example, if a class expects an argument of type String, but you pass it an Integer, we print both the expected and given types:

  Class[One]: parameter 'a' expects a String value, got Integer
  

• Could you use Puppet.deprecation_warning instead of Puppet.warning so that the deprecation (along with all other deprecations) can be suppressed using puppet apply --disable_warnings deprecations ...?

• Technically, partial argument matches are deprecated now (as of this PR) and the functionality will be removed in a future release. For example, this is the message we print for deprecated puppet applications:

  puppet/lib/puppet/application/face_base.rb

  Line 255 in e9bccae

  Puppet.deprecation_warning(_("'puppet %{face}' is deprecated and will be removed in a future release") % { face: @face.name })