command: Always validate workspace name #25262
Conversation
Codecov Report
|
mildwonkey
left a comment
mildwonkey
left a comment
There was a problem hiding this comment.
Looks good to me! I left one very small question but to be clear it isn't a merge blocker.
| return current | ||
| func (m *Meta) Workspace() (string, error) { | ||
| current, overridden := m.WorkspaceOverridden() | ||
| if overridden && !validWorkspaceName(current) { |
There was a problem hiding this comment.
I'm curious why you're only checking if the workspace name is valid when it's the result of an override? (I presume it's checked elsewhere/before this runs, but if so that might be a helpful comment!)
There was a problem hiding this comment.
This is an extremely good question, and it highlights that I haven't thought this all the way through yet! 🙏
My concern here was that because of this bug, users may have found themselves with an invalid workspace name. I wanted to make sure that they had a way out of this situation, so if the invalid workspace is selected via the .terraform/environment file, I didn't want to throw an error here.
That said, this should at the minimum have a test. And (assuming this path is even the right one) I think it should probably also result in changes to terraform workspace select and delete so that users can deal with any invalid workspaces they've already created.
I'll think this through further and come up with some next steps. But if all this sounds like a bad idea please do let me know.
There was a problem hiding this comment.
Here's what I arrived at:
- 7f51be8 adds a test for this branch, which hopefully explains the intent in code. Having an invalid workspace name currently selected (using the
.terraform/environmentfile) shouldn't break everything by returning an error fromMeta.Workspace. - 512878a ensures that users can delete workspaces which have an invalid name. This really should only have happened as a result of TF_WORKSPACE allows workspace names 'terraform workspace' does not #24564, but it seems to me that we needn't validate workspace name on delete anyway—it's closing the barn door after the horse has been badly named.
Does this all make sense?
There was a problem hiding this comment.
it's closing the barn door after the horse has been badly named.
😂 😂 😂 👍
|
I don't currently intend to make any further changes to this (unless there are more review comments), but since we're approaching an 0.13.0 release candidate, I plan to delay the merge until after 0.13.0 final is released. |
The workspace name can be overridden by setting a TF_WORKSPACE environment variable. If this is done, we should still validate the resulting workspace name; otherwise, we could end up with an invalid and unselectable workspace. This change updates the Meta.Workspace function to return an error, and handles that error wherever necessary.
We are validating the workspace name for all workspace commands. Due to a bug with the TF_WORKSPACE environment variable, it has been possible to accidentally create a workspace with an invalid name. This commit removes the valid workspace name check for workspace delete to allow users to clean up any invalid workspaces.
If somehow an invalid workspace has been selected, the Meta.Workspace method should not return an error, to ensure that we don't break any existing workflows with invalid workspace names.
alisdair
force-pushed
the
alisdair/always-validate-workspace-name
branch
from
7f51be8 to
96d2265
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. |
ghost
locked as resolved and limited conversation to collaborators
2 participants
The workspace name can be overridden by setting a
TF_WORKSPACEenvironment variable. If this is done, we should still validate the resulting workspace name; otherwise, we could end up with an invalid and unselectable workspace.This change updates the
Meta.Workspacefunction to return an error, and handles that error wherever necessary.Fixes #24564
Screenshot