Skip to content

[ci] Quote uses of path variables since it may contain spaces#10306

Closed
ycombinator wants to merge 14 commits intoelastic:masterfrom
ycombinator:quote-docker-volume
Closed

[ci] Quote uses of path variables since it may contain spaces#10306
ycombinator wants to merge 14 commits intoelastic:masterfrom
ycombinator:quote-docker-volume

Conversation

@ycombinator
Copy link
Copy Markdown
Contributor

@ycombinator ycombinator commented Jan 23, 2019 •
edited
Loading

Our Jenkins CI setup recently introduced configuration changes that cause " && " to be included in the workspace path. This is causing failures like:

15:36:55 # Change ownership of all files inside /build folder from root/root to current user/group
15:36:55 docker run -v /var/lib/jenkins/workspace/elastic+beats+pull-request+multijob-linux/beat/auditbeat/label/linux && immutable/src/github.com/elastic/beats/auditbeat:/beat alpine:3.4 sh -c "find /beat -user 0 -exec chown -h 1126:1127 {} \;"
15:36:55 "docker run" requires at least 1 argument.
15:36:55 See 'docker run --help'.
15:36:55 
15:36:55 Usage:  docker run [OPTIONS] IMAGE [COMMAND] [ARG...]
15:36:55 
15:36:55 Run a command in a new container
15:36:55 make: *** [fix-permissions] Error 1

This PR attempts to quote several paths used by our CI jobs so as to preserve the " && " in the paths as-is, and not be interpreted.

@ycombinator ycombinator requested a review from a team as a code owner January 23, 2019 23:58
@ycombinator ycombinator changed the title [ci] Quote the docker volume [ci] Quote uses of PWD since it may contain spaces Jan 24, 2019
@andrewkroh
Copy link
Copy Markdown
Member

andrewkroh commented Jan 24, 2019

There are more usages for $PWD in some of the other Makefiles. I suggest grep all the Makefiles to find other places that need updated.

andrewkroh
andrewkroh reviewed Jan 24, 2019
View reviewed changes
@ycombinator ycombinator changed the title [ci] Quote uses of PWD since it may contain spaces [ci] Quote uses of path variables since it may contain spaces Jan 24, 2019
@ycombinator
Copy link
Copy Markdown
Contributor Author

ycombinator commented Jan 24, 2019

I see the double quoting issue causing the latest failures. I just need to run a couple of errands over the next couple hours so will get back to this PR after that.

@webmat webmat mentioned this pull request Jan 24, 2019
Merged
8 tasks
andrewkroh
andrewkroh reviewed Jan 24, 2019
View reviewed changes
generator/beat/{beat}/Makefile Outdated
BEAT_NAME={beat}
BEAT_PATH={beat_path}
BEAT_GOPATH=$(firstword $(subst :, ,${GOPATH}))
BEAT_GOPATH=$(firstword $(subst :, ,"${GOPATH}"))
Copy link
Copy Markdown
Member

@andrewkroh andrewkroh Jan 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the stuff that gets passed to the shell will need quoting. So it's probably best to only quote on the shell side (where the variables are used) for consistency. Otherwise you'll end up with double quoting.

For example

TEST="TEST"

echo:
    echo "${TEST}"

$ make echo
echo ""TEST"" <- The executed command has double quotes.

@ycombinator
Copy link
Copy Markdown
Contributor Author

ycombinator commented Jan 24, 2019

@andrewkroh I've tried a few different ideas to preserve spaces in the BEAT_GOPATH variable but the build keeps failing on this error:

bash: /var/lib/jenkins/workspace/elastic+beats+pull-request+multijob-linux/beat/filebeat/label/linux/bin/gotestcover: No such file or directory

That's probably because gotestcover exists under /var/lib/jenkins/workspace/elastic+beats+pull-request+multijob-linux/beat/filebeat/label/linux && && immutable/bin/gotestcover.

I'm running out of ideas on how to preserve the space in the gotestcover path. Happy to hear ideas or maybe its time we ask Infra to use something other than the && in their workspace path?

@ycombinator
Copy link
Copy Markdown
Contributor Author

ycombinator commented Jan 24, 2019

Closing this PR unmerged for now as the original Infra change was reverted for now.

@ycombinator ycombinator deleted the quote-docker-volume branch January 24, 2019 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewkroh andrewkroh andrewkroh left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ycombinator @andrewkroh