-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Conversation
29a779e
to
1a67f7f
Compare
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!
1a67f7f
to
95fc0e6
Compare
There was a problem hiding this 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.
backend/backend.go
Outdated
"You can create a new workspace wth the \"workspace new\" command") | ||
|
||
// ErrOperationNotSupported is returned when an unsupported operation | ||
// is detecte by the configured backend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"detected"
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented code?
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
95fc0e6
to
ee11ef8
Compare
ee11ef8
to
7fb2d1b
Compare
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. |
This new backend is called
remote
and is the second natively supported enhanced backend next to the existinglocal
backend.As its name implies, where the
local
backend stores state and runs operations locally theremote
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.