-
Notifications
You must be signed in to change notification settings - Fork 103
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
Add new workspace fields #192
Conversation
b96b547
to
eaf1a09
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.
Pre-emptively ✅ this for GA.
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.
Heyo! I know this isn't quite ready for review yet, but thought I'd drop some notes because I've done a bit more thinking on this given the current API regarding the README and what I think the interface should be here in the best interests of the client.
I believe the Readme()
function (which should be in the Workspaces
interface, by the way) should make a separate call to fetch the README, and should return an io.Reader
. This is consistent with the API elsewhere, like fetching plan/apply logs or a run's JSON formatted execution plan.
Note that the current API being non-streamable isn't the point here - it's about the fact that this interface could be streamed with a separate endpoint in the future and no backwards incompatible change in type here in the client. In other words, although not useful at this immediate moment, returning an io.Reader
here doesn't hurt anything and just future proofs it for an API more appropriate for the client. Today, the API call to fulfill this interface is indeed just a nearly-duplicated /workspaces/:id?include=readme
call, but it's only out of current circumstance 👍🏻
0fbe7bc
to
0b8d2e8
Compare
I think I got to your feedback @chrisarcand . LMK |
I missed this the first time. Can you elaborate on this? I must have misunderstood you when we spoke about this. The readme doesn't really seem to belong to workspaces, it seems like it belongs to a workspace. In the case where we move the readme out of the workspace type and into say |
Sure thing: This client is organized as a set of interfaces tied to API calls. The plural name As a simpler example to grok than workspaces, check out plan.go: Lines 19 to 25 in 1de9e44
Lines 48 to 61 in 1de9e44
Note that to retrieve the logs for a plan, is not a method on In your case, this means you should:
|
f70a92f
to
5e86452
Compare
a6c39e7
to
b4c11cd
Compare
b4c11cd
to
b0a5cbd
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.
lgtm
This requires a bit of fiddling with our CI to be green, and I'll get to that today 👍🏻 |
Description
Adds support for new fields in the workspace object
Example Output
Testing plan
Added a unit test, but it won't work because this feature is not generally available yet. It should pass once it is.
The fields have been tested manually against an existing workspace with real data.