Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(maint) Fix expect_upload for module relative paths outside files #3238

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jpartlow
Copy link
Contributor

The expect_upload() BoltSpec expectation for upload_file calls in plans handled two cases:

Given a /my/modules/amodule with

/my/modules/amodule/files/foo
/my/modules/amodule/resources/baz
/my/modules/amodule/plans/bar

expect_upload('/some/thing') would correctly match against an upload_file('/some/thing') in plan bar, because it is not relative to the modulepath at all, and against an upload_file('amodule/foo'), because it is relative to amodule, and foo is relative to files, which expect_upload elides when the MockExecutor runs the source_path through BotlSpec::MockExecutor#module_file_id.

The edge case that was peculiar was a call to
upload_file('amodule/files/../resources/baz'), which expect_upload would end up expecting as 'amodule/baz'.

The hidden behavior here is that Bolt::Util.find_file* functionality will take 'foo/bar' and look for /modulepath/foo/files/bar, which is mirroring Puppety behavior to find module files relative to the files/ dir.

@jpartlow jpartlow requested a review from a team as a code owner September 29, 2023 20:11
@jpartlow
Copy link
Contributor Author

I'll deal with my offenses to the linting gods; but I'm not entirely certain about this patch either way.

Even an expect_upload('amodule/resources/bar') is a bit odd, since you'd be inclined to think it was really 'amodule/files/resources/bar'. Not sure what the ideal behavior is here or if this is worth patching.

The expect_upload() BoltSpec expectation for upload_file calls in plans
handled two cases:

Given a /my/modules/amodule with

  /my/modules/amodule/files/foo
  /my/modules/amodule/resources/baz
  /my/modules/amodule/plans/bar

expect_upload('/some/thing') would correctly match against an
upload_file('/some/thing') in plan bar, because it is not relative to
the modulepath at all, and against an upload_file('amodule/foo'),
because it is relative to amodule, and foo is relative to files,
which expect_upload elides when the MockExecutor runs the source_path
through BotlSpec::MockExecutor#module_file_id.

The edge case that was peculiar was a call to
upload_file('amodule/files/../resources/baz'), which expect_upload
would end up expecting as 'amodule/baz'.

The hidden behavior here is that Bolt::Util.find_file* functionality
will take 'foo/bar' and look for /modulepath/foo/files/bar, which is
mirroring Puppety behavior to find module files relative to the files/
dir.
@jpartlow jpartlow force-pushed the maint-fix-expect-upload-for-module-relative-outside-files branch from 6a424e4 to 19f0469 Compare September 29, 2023 20:37
@jpartlow
Copy link
Contributor Author

jpartlow commented Sep 29, 2023

Would also need some fiddling to make the module_file_id tests OS agnostic, or drop them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant