Hi, I'm attempting to build terraform from source using make. In fact here's a handy Dockerfile to completely demonstrate the failure:
FROM ubuntu:latest
RUN apt-get update && apt-get install -y git curl wget golang build-essential
RUN bash -c 'mkdir -p /opt/go/src/github.com/{hashicorp,mitchellh}'
ENV GOPATH=/opt/go
ENV PATH=/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/opt/go/bin
RUN ls /opt/go/src/github.com/
RUN cd /opt/go/src/github.com/hashicorp \
&& git clone https://github.com/hashicorp/terraform \
&& cd terraform \
&& git checkout v0.6.16 \
&& make
This always fails with:
--- FAIL: TestRead_PathNoPermission (0.00s)
read_test.go:82: Expected error, got none!
FAIL
FAIL github.com/hashicorp/terraform/helper/pathorcontents 0.003s
Is this test broken? It also fails when checking out master instead of v0.6.16. Thanks
The cause was building as root, as root can read files chmodded as 000, thus causing the test to fail.
Probably in the prerequisites, documentation, it should be noted that an unprivileged user has to run "make".
Question: is there any ongoing relevance or reason to keep this particular test as part of the Terraform suite: it tests some basic filesystem semantics that happen to break down under Docker (and the overall trend for build/CI solutions is to be backed by Docker or docker-like setups), and special-casing the test to skip under Docker means the test probably isn't adding much.
Why not drop the test? All it's doing is writing a file, taking away permissions and then trying to read it back - and it's entirely just combining calls to core library code, and not even touching TF code. That's the kind of behavioural testing you'd expect to see inside the golang io package - and not in it's consumers. In fact, the only test I can see being overly useful in that file is actually the one that tests the resolution of ~/ paths cross-platform using the imported library.
@phinze , any thoughts on comments from @steve-gray regarding the TestRead_PathNoPermission.
The test complicates things for those of us working with the terraform code base in a Docker container and doesn't seem to provide any critical test coverage.
Hey folks! I had some fun trying to figure out what 3-years-ago @phinze was thinking there. Here's what I've come up with:
TestRead_PathNoPermission verifies that the pathorcontents.Read helper function properly returns an error when a path exists but is not readable. It is the test that covers the implementation's extra os.Stat before diving in to read the file.
Without this, the helper doesn't have its present-but-not-readable behavior specified one way or another, and an implementation that simply returned the path and no error for an unreadable path would pass all the other tests.
In other words, I think the test is worth keeping. I'd be fine for us to t.Skip it if the user is root though.
Hope this helps explain the context here!
Makes sense, thanks for looking into this @phinze and chasing down what 3-years-ago @phinze was doing. Also appreciate the PR to harden the test.
I'm going to lock this issue because it has been closed for _30 days_ โณ. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.
Most helpful comment
Question: is there any ongoing relevance or reason to keep this particular test as part of the Terraform suite: it tests some basic filesystem semantics that happen to break down under Docker (and the overall trend for build/CI solutions is to be backed by Docker or docker-like setups), and special-casing the test to skip under Docker means the test probably isn't adding much.
Why not drop the test? All it's doing is writing a file, taking away permissions and then trying to read it back - and it's entirely just combining calls to core library code, and not even touching TF code. That's the kind of behavioural testing you'd expect to see inside the golang io package - and not in it's consumers. In fact, the only test I can see being overly useful in that file is actually the one that tests the resolution of ~/ paths cross-platform using the imported library.