-
Notifications
You must be signed in to change notification settings - Fork 156
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 Source Name and URL #527
Conversation
Addresses hashicorp#392 The TFE SDK does actually contain `SourceName` and `SourceURL` fields already: https://github.com/hashicorp/go-tfe/blob/main/workspace.go#L129-L130 This is currently untested
`tfe.WorkspaceUpdateOptions` does not contain these fields, which makes sense, as those fields can only be set upon workspace creation. Will need to test to see what happens when a workspace is updated to add these fields.
Additionally, if SourceURL is set, but SourceName is not, set Name to URL
Ah, it's failing on:
because agent pools are a paid feature. With
so not sure if that's a flakey test, or something else up |
Added some tests specific to this feature:
And it looks like there's actually a problem with my own functionality too, specifically how it handles the case where (I might be trying to overcomplicate it actually; perhaps it's better not to try to be clever, and instead require that if either 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.
This looks good overall. Might want to have someone that writes providers frequently review it just in case I missed something. I left a few wording suggestions and some general comments. Nice work!
Good catches! This is what happens when I finish off a PR while half-asleep on a train and procrastinating from the work I'm actually supposed to be doing 🤣 I'll get those added. |
Co-authored-by: Matthew Sanabria <24284972+sudomateo@users.noreply.github.com>
Co-authored-by: Matthew Sanabria <24284972+sudomateo@users.noreply.github.com>
Changelog updated to fix conflicts |
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.
💖 Thank you for submitting this PR! We'll wait to merge this until the attributes are in GA.
Added the label |
That’s fair. I suspected we might want to do that, just in case we change how they work. 🙂 Do we have any timeline on when we expect those to be GA? |
I'm finding out if we can merge this PR. Stay tuned! 📻 |
Oh nice! We'd need to resolve the conflict of course, but if this is likely to get merged that'd be awesome :) |
Conflicts should be resolved. I'm gonna do a quick once-over in the diff to make sure it didn't break anything, then it should be all ready to go. Edit: Some auto-checks failed, due to changes in the code since I originally raised the PR. Getting those resolved, and then I'll let you know when it's ready. Edit 2: Well... it's still failing, but as far as I can tell that's not due to anything in my change. |
52d2ab9
to
f97eae5
Compare
It has had a new field added since I initially raised the PR
Small update: we're sorting out a build/CI issue. |
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.
#783 for the version of this with working CI |
Description
Addresses #392
Testing plan
Example code:
(See #392 for further tests)
External links
Output from acceptance tests
Please run applicable acceptance tests locally and include the output here. See TESTS.md to learn how to run acceptance tests.
If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.
i.e. there was a failure... but not in resources I've modified