-
Notifications
You must be signed in to change notification settings - Fork 157
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
Workspace working directory updates #137
Workspace working directory updates #137
Conversation
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.
Looks good, but it does need a small change before we can merge it.
Output of new added dedicated test for working directory update:
Reverted my changes for fix of
|
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.
Nice @etopeter! But it seems this introduces some backwards incompatible behavior...
Thought about backwards compatibility
I just thought about the possible impact for existing users of this provider and I think this change introduces a backwards incompatibility and/or risk. If you created workspaces previously without setting a workspace in your TF config, and over time you manually (or by any other means, but not using TF) updated your working directories they will now all be reset to the root directory. Not sure what would be a good mitigation, but good to think about this before merging this PR. |
@svanharmelen The only way I can think of to have this backward compatible is to change attribute name and support both. Use of one would exclude another. We could name it In my opinion current behavior of plugin is buggy. You can set working directory but you can't change it to root once it was set to something. You always have to have non null value for changes to be detected. Why I think we should use proposed changes: What could happen? |
@etopeter I fully agree and also don't think it will have a big impact, yet we need to be careful with backwards incompatible changes of course. @radeksimko @beekus could you maybe step in here? Not sure who currently has ownership/responsibility of this provider within HashiCorp, but I guess they should give their input on this one. |
Hey @svanharmelen and @etopeter Thanks for your work on this! @nicsnet happens to be working on a very similar issue related to trigger prefixes for workspaces. These two PRs together fill in some gaps we've had in the provider nicely. As these two PRs are merged, we'll be sure to call out in the changelog the important note that "if you have workspaces which are configured through the TFE provider, but have set the working directory or trigger prefixes manually, through the UI, you'll need to update your configuration". In the case where the user does not update their configuration after pulling in this provider change, they'll still be informed of the drift when they run the Plan step. |
Description
Added support for changing working directory for existing workspaces.
Testing plan
If you create workspace with defined working directory and after apply you try to change it, provider will show there are no changes to be done. (working directory is computed value)
Issue reference #136
Output from acceptance tests
Changes were made to test
TestAccTFEWorkspace_update