(PUP-4629) Return values instead of augeas paths during the match operation by mmarod · Pull Request #3952 · puppetlabs/puppet

mmarod · 2015-05-19T15:47:09Z

This fixes PUP-4629 by using aug.get to return the values of the paths returned by the aug.match operation.

puppetcla · 2015-05-19T17:00:21Z

CLA signed by all contributors.

peterhuene · 2015-05-19T19:03:42Z

Hi @mmarod. Thanks for the contribution!

@lutter and @domcleal: as you're both more knowledgeable about the augeas provider than I am, does this change make sense to either of you?

Note to reviewers: the AppVeyor failure is the known rdoc failure and can be ignored.

domcleal · 2015-05-20T06:42:57Z

I can see where you're going with this, but changing the behaviour of match isn't a good idea as it could easily break existing manifests (and is different to what aug_match actually does).

I would suggest adding a get command with this behaviour instead (although in Augeas' API, get won't work for multiple nodes, but having it do so here might be OK).

domcleal · 2015-05-20T06:43:58Z

I would suggest adding a get command with this behaviour instead

Oh, we do have it, apologies.

Maybe call it something else then if you'd like support for returning an array of values, e.g. getm (to emulate setm, clearm etc.)

mmarod · 2015-05-20T15:34:08Z

Adding a getm seems reasonable but it seems like it might be unnecessary. It boils down to what the intent of the match <MATCH_PATH> == <AN_ARRAY> is really meant to do. Is it supposed to compare labels or values?

I would argue that the match <MATCH_PATH> != <AN_ARRAY> and match <MATCH_PATH> == <AN_ARRAY> controls in onlyif do not currently do what the Puppet type reference says they should do. AN_ARRAY is described as ['a string, 'another'] which could never actually be returned when calling aug.match. It would instead return ['/files/something/some_param[1]', '/files/something/some_param[2]'] which is not useful to match against.

If we are trying to make the controls in onlyif mimic exacty what augeas does, then we should add a getm parameter. However, it seems to me (according to the type ref) that match <MATCH_PATH> == <AN_ARRAY> was meant to solve this problem in the first place.

domcleal · 2015-05-20T15:39:03Z

I'd say that's a documentation bug and that changing the provider behaviour is a) going to be incompatible, so would have to live in master for some time, and b) would differ from Augeas.

I think mimicking Augeas' API is quite important, as many users will test how Augeas responds via augtool, so to change the return value from a standard API call or command is surprising.

This is a very useful feature, particularly so the == and include operators work more naturally, but I don't think changing an existing command is the way to do it.

mmarod · 2015-05-20T15:41:02Z

Fair enough

mmarod · 2015-05-20T16:15:29Z

If we are trying to mimic augeas commands exactly -- how should getm be implemented? There does not seem to be a built in getm command. It seems running a match and then passing each result into a get is the only way to retrieve all of the values in an augeas path.

domcleal · 2015-05-21T07:40:33Z

Yes, that'd be the way to do it.

peterhuene · 2015-05-21T17:12:55Z

Hi @mmarod Thanks for pushing up a new commit! Would you mind squashing the two commits into one (basically amending the original commit)?

domcleal · 2015-05-22T07:14:34Z

spec/unit/provider/augeas/augeas_spec.rb

It doesn't affect the code, but for clarity, change match to getm. (Ditto below.)

domcleal · 2015-05-22T07:19:02Z

Looks good to me. Would you also mind adding the include and not_include operators to getm while we're here? I think they're likely to be useful.

domcleal · 2015-05-22T15:16:36Z

👍 looks good, and tests well.

domcleal · 2015-05-22T15:25:23Z

Oh, the only thing I'd add is that the commit message doesn't quite reflect what it does any more, since the match behaviour's been left unchanged. Would you mind updating that?

lutter · 2015-05-22T17:57:20Z

I am a little late to the party, but how about calling that new operator values ? That seems to express pretty well what it's supposed to do, and there won't be any way to confuse it with existing augtool commands.

mmarod · 2015-05-22T18:57:20Z

values seems a bit less confusing to me. Since getm isn't a true augeas API call anyway it shouldn't make a difference whether we call it getm or values. What do you think @domcleal ?

domcleal · 2015-05-24T17:57:07Z

Good suggestion, works for me!

peterhuene · 2015-05-26T20:40:48Z

@domcleal and @lutter: could either one of you review @mmarod's changes one last time to make sure this looks good (as I'm no augeas provider expert) before we merge this? Thanks!

domcleal · 2015-05-27T07:27:47Z

Looks fine to me, although the commit message is incorrect again, s/getm/values/. Could be fixed on merge.

mmarod · 2015-05-27T13:31:10Z

Fixed the commit message again

domcleal · 2015-05-27T13:33:23Z

👍 thanks @mmarod!

ffrank · 2015-06-02T17:15:59Z

lib/puppet/provider/augeas/augeas.rb

Could this loop be replaced by an invocation of Array#map aka #collect?

Good idea... I updated it

joshcooper · 2015-06-04T17:06:28Z

The appveyor failures are a known issue https://tickets.puppetlabs.com/browse/PUP-4537 and can be ignored.

mmarod · 2015-06-25T18:26:29Z

So it's been a while since this is seemingly ready to go -- What is the hold up?

joshcooper · 2015-06-25T19:10:28Z

@mmarod We've had to cancel the last three community PR triage sessions while preparing for the Puppet 4.2.0 release. We'll resume next 10am PDT, Wednesday, June 1, so it should be merged soon.

joshcooper · 2015-06-30T00:24:34Z

@mmarod one minor nit, could you update the commit message to describe how values can be used as a result of this change? A real-world example would be really useful. Thank you!

joshcooper · 2015-07-06T19:01:59Z

@mmarod thanks for updating that, the example is very helpful. Could you shorten the subject line to follow our convention as described in https://github.com/puppetlabs/puppet/blob/master/CONTRIBUTING.md:

(PUP-1234) Make the example in CONTRIBUTING imperative and concrete

Without this patch applied the example commit message in the CONTRIBUTING
document is not a concrete example.

I'm happy to fix that up if you're busy. Thank you again!

mmarod · 2015-07-06T19:44:22Z

No problem -- Let me know how it looks.

This patch allows comparisons against values. Previously, comparisons could only be performed against labels. Example usage would be to ensure that augeas only runs if the values of cfg_file using the Nagios lens do not match a list of known values. augeas { 'configure-nagios-cfg_file': incl => '/etc/nagios/nagios.cfg', lens => 'NagiosCfg.lns', changes => [ "rm cfg_file", "ins cfg_file", "set cfg_file[1] /etc/nagios/commands.cfg", "ins cfg_file after /files/etc/nagios/nagios.cfg/cfg_file[last()]", "set cfg_file[2] /etc/nagios/anotherconfig.cfg" ], onlyif => "values cfg_file != ['/etc/nagios/commands.cfg', '/etc/nagios/anotherconfig.cfg']" }

joshcooper · 2015-07-08T16:20:46Z

Thank you @mmarod for your contribution! This will be released in puppet 4.3.0.

peterhuene added the Community label May 19, 2015

mmarod force-pushed the fix/master/augeas_match_use_value branch from 8aa8fe2 to a37cb80 Compare May 19, 2015 15:53

mmarod force-pushed the fix/master/augeas_match_use_value branch from a37cb80 to 1f64f39 Compare May 19, 2015 17:54

peterhuene added the Triaged label May 19, 2015

mmarod force-pushed the fix/master/augeas_match_use_value branch 2 times, most recently from 1528450 to 14c5e4b Compare May 21, 2015 17:27

domcleal reviewed May 22, 2015
View reviewed changes

mmarod force-pushed the fix/master/augeas_match_use_value branch 2 times, most recently from 37fac9a to 3b9f816 Compare May 22, 2015 14:26

domcleal added the Client label May 22, 2015

mmarod force-pushed the fix/master/augeas_match_use_value branch from 3b9f816 to 04a21c7 Compare May 22, 2015 15:34

mmarod force-pushed the fix/master/augeas_match_use_value branch from 9652880 to 58f52cf Compare May 25, 2015 22:00

mmarod force-pushed the fix/master/augeas_match_use_value branch from 58f52cf to 35cee76 Compare May 27, 2015 13:30

ffrank reviewed Jun 2, 2015
View reviewed changes

mmarod force-pushed the fix/master/augeas_match_use_value branch from dc0199b to 7ef9afd Compare June 2, 2015 20:53

joshcooper added the Merge After Unfreeze label Jun 29, 2015

mmarod force-pushed the fix/master/augeas_match_use_value branch from 7ef9afd to 1c8c87a Compare June 30, 2015 20:47

mmarod force-pushed the fix/master/augeas_match_use_value branch from 1c8c87a to f7e35ac Compare July 6, 2015 19:43

mmarod force-pushed the fix/master/augeas_match_use_value branch from f7e35ac to 81f2378 Compare July 6, 2015 19:46

mmarod force-pushed the fix/master/augeas_match_use_value branch from 81f2378 to f8ddf88 Compare July 6, 2015 19:47

joshcooper merged commit f8ddf88 into puppetlabs:master Jul 8, 2015

Conversation

mmarod commented May 19, 2015

Uh oh!

puppetcla commented May 19, 2015

Uh oh!

peterhuene commented May 19, 2015

Uh oh!

domcleal commented May 20, 2015

Uh oh!

domcleal commented May 20, 2015

Uh oh!

mmarod commented May 20, 2015

Uh oh!

domcleal commented May 20, 2015

Uh oh!

mmarod commented May 20, 2015

Uh oh!

mmarod commented May 20, 2015

Uh oh!

domcleal commented May 21, 2015

Uh oh!

peterhuene commented May 21, 2015

Uh oh!

domcleal May 22, 2015

Choose a reason for hiding this comment

Uh oh!

domcleal commented May 22, 2015

Uh oh!

domcleal commented May 22, 2015

Uh oh!

domcleal commented May 22, 2015

Uh oh!

lutter commented May 22, 2015

Uh oh!

mmarod commented May 22, 2015

Uh oh!

domcleal commented May 24, 2015

Uh oh!

peterhuene commented May 26, 2015

Uh oh!

domcleal commented May 27, 2015

Uh oh!

mmarod commented May 27, 2015

Uh oh!

domcleal commented May 27, 2015

Uh oh!

ffrank Jun 2, 2015

Choose a reason for hiding this comment

Uh oh!

mmarod Jun 2, 2015

Choose a reason for hiding this comment

Uh oh!

joshcooper commented Jun 4, 2015

Uh oh!

mmarod commented Jun 25, 2015

Uh oh!

joshcooper commented Jun 25, 2015

Uh oh!

joshcooper commented Jun 30, 2015

Uh oh!

joshcooper commented Jul 6, 2015

Uh oh!

mmarod commented Jul 6, 2015

Uh oh!

joshcooper commented Jul 8, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants