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

Try to find a workspace by external ID before removing it #51

Merged
merged 2 commits into from Jan 17, 2019

Conversation

svanharmelen
Copy link
Contributor

When a workspace name is changed out side of TF, we should still be able
to find it using it's external ID. So we first try that before we delete
the workspace from the state.

When a workspace name is changed out side of TF, we should still be able
to find it using it's external ID. So we first try that before we delete
the workspace from the state.
Copy link

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

Ideally we'd see a test for exercising this code, but looks good to me.

@svanharmelen
Copy link
Contributor Author

@paddycarver I fully agree, but this one is kind of hard to test. The use case for this is when someone changes the name of the workspace outside of the use of TF. So given our current test harnas I'm not sure how I can/should test that?

The only thing I can come up with is a custom function that uses direct API calls to change the name as a test step. But that feels ... nasty. What do you think?

@paddycarver
Copy link

The only thing I can come up with is a custom function that uses direct API calls to change the name as a test step. But that feels ... nasty. What do you think?

That's generally how we do it in the Google provide, writing a custom resource.TestStep or TestCheck that manipulates the resource using direct API calls. YMMV.

@svanharmelen
Copy link
Contributor Author

Well in that case, I'll have look at it 👍 I do miss the test here myself as well, so if we already use that pattern elsewhere I'm fine with using it here as well...

@ghost ghost added size/M and removed size/S labels Jan 17, 2019
@svanharmelen
Copy link
Contributor Author

@paddycarver just added a test, would be nice if you can double check it before I merge the PR.

@svanharmelen svanharmelen merged commit 9df993a into master Jan 17, 2019
@svanharmelen svanharmelen deleted the svh/f-workspaces branch January 17, 2019 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants