[DEVEX-1216] context commands can now be used with --org-id parameter.
#1046
Conversation
There was a problem hiding this comment.
Alright, the design seems great to me.
However I notice a pattern of using pointers a lot. Is there a justification for this that I'm missing? If it's to mark presence or absence of the variable, most of the time, the zero value works just as well, especially with string, where empty string often works just as well as nil (except in cases where empty string makes semantic sense).
If there's a justification, sorry for all the comments, let's discuss so I can understand better.
api/rest/client.go
Outdated
| @@ -129,6 +129,10 @@ func (c *Client) DoRequest(req *http.Request, resp interface{}) (int, error) { | |||
| return httpResp.StatusCode, nil | |||
| } | |||
|
|
|||
| func (c *Client) Do(req *http.Request) (*http.Response, error) { | |||
There was a problem hiding this comment.
I know it's not your fault that rest.Client is already a leaky abstraction, but it seems that wherever you call Do, you're doing things very similarly to DoRequest.
Is there a way to re-use, maybe change, the existing DoRequest method, rather than creating a new, leakier method?
api/context/rest_client.go
Outdated
| } | ||
|
|
||
| // createListContextsParams is a helper to create ListContextsParams from the content of the ContextRestClient | ||
| func (c *restClient) createListContextsParams() (*ListContextsWithRestParams, error) { |
There was a problem hiding this comment.
Why return a pointer, here?
There was a problem hiding this comment.
I think, as a Go practice I like to return a unusable value in case of error, returning a pointer here allow to return nil in case of error.
api/context/rest.go
Outdated
| type ListEnvVarsWithRestParams struct { | ||
| ContextID string | ||
| PageToken *string | ||
| } | ||
|
|
||
| type listEnvVarsResponse struct { | ||
| Items []EnvironmentVariable `json:"items"` | ||
| NextPageToken *string `json:"next_page_token"` | ||
| } |
There was a problem hiding this comment.
I think even your page tokens could be defined as string. In many cases, checking against "" works just as well as checking against nil, with less complexity.
JulesFaucherre
force-pushed
the
DEVEX-1216
branch
5 times, most recently
from
6891a11
to
42785df
Compare
api/context/rest.go
Outdated
| NextPageToken string `json:"next_page_token"` | ||
| } | ||
|
|
||
| func ListEnvVarsWithRest(client *rest.Client, params ListEnvVarsWithRestParams) (*listEnvVarsResponse, error) { |
There was a problem hiding this comment.
You could generalize for all return values. No need to return a pointer if you don't have anything hard to copy in the struct (like a mutex, a file or a network connection), or if you know the copying is a performance bottleneck. In many cases, not making it a pointer even lets the compiler do some cool optimizations.
Also, I think the return type should be exported, since it's returned from an exported func, and has exported fields.
| func ListEnvVarsWithRest(client *rest.Client, params ListEnvVarsWithRestParams) (*listEnvVarsResponse, error) { | |
| func ListEnvVarsWithRest(client *rest.Client, params ListEnvVarsWithRestParams) (ListEnvVarsResponse, error) { |
There was a problem hiding this comment.
This comment has sent me down a rabbit hole of when to:
- use pointers as receivers
- pass pointers as arguments
- return pointers from functions
Opinions vary, and many people inject different semantics into the simple * in Go. Optionals, mutability, pass-by-reference... Very different from Rust and its explicit mut, & references and std::option, or C++ and its many types of "smart pointers". We should probably take this offline and establish our own guide of "Pointer usage in Go". Hell, we could probably make a blog post out of it.
Do as you like for pointer types. You can resolve this, let's chat later.
There was a problem hiding this comment.
Nice, I don't know but maybe someone in CCI wrote some go practices for the company.
Refactored the context api abstraction. You can now access function to request contexts directly or use an abstraction that handles server without v2 API.
JulesFaucherre
force-pushed
the
DEVEX-1216
branch
from
42785df
to
dba46aa
Compare
Jira ticket
Changes
orgIDorvcsType / orgNameto reference an org. The information is passed upon creation of the context API client.circleci contextto use the--org-id.circleci context store-secret,circleci context remove-secret,circleci context createandcircleci context deleteshow a text line upon success to inform user.Rationale
Having these commands use the
--org-idparameter was necessary to support standalone projects.Considerations
The context API had to be adapted to handle using
orgIDinstead ofvcsType / orgName. A first approach was already implemented forCreateContext. The solution taken here was to add a methodCreateContextWithOrgID.After some reflexion, we thought that duplicating every call of the interface would not be very clear and would pass the decision of whether to use
orgIDorvcsType / orgNameto the caller. Thus we decided to unite the interface into one that would be instantiated with the information about its org.Screenshots
N/A.