(#9440) exec resource: set path param to path fact #9445
Conversation
|
Can one of the admins verify this patch? |
bastelfreak
force-pushed
the
issue-9440
branch
3 times, most recently
from
8a1f94c
to
fba26a5
Compare
| @@ -650,32 +650,6 @@ def with_another_agent_running(&block) | |||
| .and output(/No more routes to fileserver/).to_stderr | |||
| end | |||
|
|
|||
| it 'preserves the old cached catalog if validation fails with the old one' do | |||
There was a problem hiding this comment.
we should probably keep this it. It used the exec resource to create a broken catalog. but with this change it's not broken anymore. I removed it for now so the tests continue.
There was a problem hiding this comment.
Maybe add path => '' to preserve the intent of the test?
There was a problem hiding this comment.
this now verifies that path => '' fails. But it doesn't verify anymore if the cached catalog isn't updated.
There was a problem hiding this comment.
ah interesting, this is because previously the agent attempted to convert the resource catalog to a RAL catalog
puppet/lib/puppet/configurer.rb
Line 485 in 26ab7e1
That caused the validate lifecycle method to be called on each type & parameter
puppet/lib/puppet/type/exec.rb
Line 589 in 26ab7e1
Previously we detected the command wasn't absolute and path was omitted, so we raised and the cache was never updated.
I think we want to update the validatecmd method https://github.com/puppetlabs/puppet/blob/26ab7e13daa4569cc62492ef882dce0b2796ef27/lib/puppet/provider/exec.rb#L105C1-L105C182
- self.fail _("'%{exe}' is not qualified and no path was specified. Please qualify the command or specify a path.") % { exe: exe } if !absolute_path?(exe) and resource[:path].nil?
+ self.fail _("'%{exe}' is not qualified and no path was specified. Please qualify the command or specify a path.") % { exe: exe } if !absolute_path?(exe) && (resource[:path].nil? || resource[:path].empty?)And then preserve the test as it was.
There was a problem hiding this comment.
I applied your suggestion and undid the test changes, but now it's failing again like it did initially. Maybe I misunderstood your 'preserve the test as it was'. Can you take another look please?
bastelfreak
force-pushed
the
issue-9440
branch
4 times, most recently
from
752a4a1
to
75a778c
Compare
There was a problem hiding this comment.
Thanks @bastelfreak! I was initially concerned this might affect the PATH for the child process, but I'm fairly certain it only affects puppet's which functionality when resolving an unqualified command. So it should be ok. I just had some minor comments about the tests.
Also could you add a test for the situation where path => '<something>' is specified and overrides the default? Or maybe there is already a test for that?
| @@ -223,7 +223,9 @@ def sync | |||
| newparam(:path) do | |||
| desc "The search path used for command execution. | |||
| Commands must be fully qualified if no path is specified. Paths | |||
| can be specified as an array or as a '#{File::PATH_SEPARATOR}' separated list." | |||
| can be specified as an array or as a '#{File::PATH_SEPARATOR}' separated list. Defaults to the `path` fact." | |||
There was a problem hiding this comment.
Unrelated to this PR but we shouldn't use PATH_SEPARATOR because reference docs are generated on Linux, so the type reference will be wrong for Windows. I think we call out "use ';' on Windows, otherwise ':' everywhere else" in other doc strings.
| @@ -650,32 +650,6 @@ def with_another_agent_running(&block) | |||
| .and output(/No more routes to fileserver/).to_stderr | |||
| end | |||
|
|
|||
| it 'preserves the old cached catalog if validation fails with the old one' do | |||
There was a problem hiding this comment.
Maybe add path => '' to preserve the intent of the test?
bastelfreak
force-pushed
the
issue-9440
branch
7 times, most recently
from
158972d
to
6a557cc
Compare
bastelfreak
force-pushed
the
issue-9440
branch
from
6a557cc
to
62e7280
Compare
|
@joshcooper can you take a look again please? |
bastelfreak
force-pushed
the
issue-9440
branch
2 times, most recently
from
56b511a
to
e2a70e8
Compare
Signed-off-by: Tim Meusel <[email protected]>
bastelfreak
force-pushed
the
issue-9440
branch
from
e2a70e8
to
3864ff8
Compare
No description provided.