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

[WIP] backend/enhanced: start with absolute configuration path #22096

Merged
merged 2 commits into from
Jul 17, 2019

Conversation

mildwonkey
Copy link
Contributor

We recently started normalizing the config path before all "command"
operations, which was necessary for consistency but had unexpected
consequences for remote backend operations, specifically when a vcs root
with a working directory are configured.

This PR de-normalizes the path back to an absolute path.

I am still contemplating tests - this issue only occurs when a TFC user has a workspace with both a VCS root and a working directory configured, not something we can cover in unit tests. Any suggestions are appreciated.

We recently started normalizing the config path before all "command"
operations, which was necessary for consistency but had unexpected
consequences for remote backend operations, specifically when a vcs root
with a working directory are configured.

This PR de-normalizes the path back to an absolute path.
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Looks good so far!

From a testing standpoint, the only thing that springs to mind is a rather heavyweight solution of having this run against a mock API server in the test and verifying that a suitable result gets submitted. Ideally it'd be nice if we could make that call to b.client.ConfigurationVersions.Upload below instead point to a mock implementation, and thus avoid the need to start up a server at all, but I believe b.client is a concrete struct from the Terraform Cloud client library and thus not something we can mock without some considerable refactoring. 😖

@svanharmelen
Copy link
Contributor

but I believe b.client is a concrete struct from the Terraform Cloud client library and thus not something we can mock without some considerable refactoring

That is not entirely true. Have a look at the testBackend function defined here. It creates a new client and then mocks endpoints that are used by the remote backend (using the code defined in backend_mock.go).

As I was running into this issue myself, I was going to take a stab at fixing the problem. But since that seems to be done already, let me take a stab at adding a test for the fix instead.

It turned out all required logic was already present, so I just needed to add a test for this specific use case.
@svanharmelen
Copy link
Contributor

@mildwonkey I added a single commit on your branch that adds an error check and a test for this use case.

It turned out that the mock and existing tests we used already worked as expected, but since they always use a specific configuration directory, the problem didn't show itself. So I added an additional test that simulates doing a plan from the current working directory which fails when running without the fix you added in this PR.

@mildwonkey
Copy link
Contributor Author

@svanharmelen thank you so much!! 🎉 I woke up annoyed that I hadn't had any brilliant ideas, and here you are 😀 I greatly appreciate the assist and I will learn from your test.

@mildwonkey mildwonkey merged commit 89eeaed into master Jul 17, 2019
@svanharmelen
Copy link
Contributor

Your welcome 😄

@ghost
Copy link

ghost commented Aug 17, 2019

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.

@ghost ghost locked and limited conversation to collaborators Aug 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants