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

backend/remote: add a new backend for storing state and running operations remotely #18596

Merged
merged 5 commits into from
Aug 3, 2018

Conversation

svanharmelen
Copy link
Contributor

This new backend is called remote and is the second natively supported enhanced backend next to the existing local backend.

As its name implies, where the local backend stores state and runs operations locally the remote backend stores state and runs operations remotely.

The supported remote endpoints must support the full TFE V2 API, so only Terraform Enterprise and Private Terraform Enterprise are currently supported endpoints.

@svanharmelen svanharmelen changed the title Add a new backend for storing state and running operations remotely core: add a new backend for storing state and running operations remotely Aug 3, 2018
@svanharmelen svanharmelen changed the title core: add a new backend for storing state and running operations remotely backend/remote: add a new backend for storing state and running operations remotely Aug 3, 2018
@svanharmelen svanharmelen force-pushed the svh/f-remote-backend branch 2 times, most recently from 29a779e to 1a67f7f Compare August 3, 2018 09:25
Sander van Harmelen added 4 commits August 3, 2018 11:29
It’s a purely cosmetic change, but I find it easier to read them like this.
By adding this method you now only have to pass a `*disco.Disco` object around in order to do discovery and use any configured credentials for the discovered hosts.

Of course you can also still pass around both a `*disco.Disco` and a `auth.CredentialsSource` object if there is a need or a reason for that!
@apparentlymart apparentlymart requested review from a team and removed request for apparentlymart August 3, 2018 17:20
Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

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

This looks great! I left a few minor notes in there, but nothing blocking merging.

"You can create a new workspace wth the \"workspace new\" command")

// ErrOperationNotSupported is returned when an unsupported operation
// is detecte by the configured backend.
Copy link
Member

Choose a reason for hiding this comment

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

"detected"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

"\n\nThe \"remote\" backend currently only supports the \"plan\" operation.\n"+
"Please use the remote backend web UI for all other operations:\n"+
"https://%s/app/%s/%s", b.hostname, b.organization, op.Workspace)
// return nil, backend.ErrOperationNotSupported
Copy link
Member

Choose a reason for hiding this comment

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

remove commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this one on purpose as its going to be something I want to pickup in the next iteration (as soon as this one is merged). So, for this one time, I would like to leave this line in...

func TestRemote_planBasic(t *testing.T) {
b := testBackendDefault(t)

mod, modCleanup := module.TestTree(t, "./test-fixtures/plan")
Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to wrap these paths in a function to set the path with the correct path separator. Not a big deal yet, but eventually we want our tests to be able to run on windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have a look, but I would say that deserves a PR on its own then (to update that everywhere). As now it would stand out if I do it here, while no other backends tests currently do that... Something with consistency 😉 yet I totally get your point 👍

Copy link
Member

Choose a reason for hiding this comment

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

Good point, consistency first 👍

func TestRemote_backendDefault(t *testing.T) {
b := testBackendDefault(t)
backend.TestBackendStates(t, b)
backend.TestBackendStateLocks(t, b, b)
Copy link
Member

Choose a reason for hiding this comment

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

I didn't catch if it's supported, but there's also a separate TestBackendStateForceUnlock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks for the pointer. I'll have a look at that Monday morning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it to the test 👍

@svanharmelen svanharmelen merged commit f4da82a into master Aug 3, 2018
@svanharmelen svanharmelen deleted the svh/f-remote-backend branch August 3, 2018 20:49
@ghost
Copy link

ghost commented Apr 2, 2020

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 Apr 2, 2020
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.

2 participants