-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add support to set the GC Stack the CLI should use by default #4940
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
base: master
Are you sure you want to change the base?
Changes from 16 commits
f2b14fd
12ff28d
2813fd8
671c401
12fc907
ff92bc4
db0134a
6f23366
2f92dc5
d048793
408f03b
f0cd139
79806d7
dc427dd
c619442
44b495a
217e011
35b4015
3f6df55
154b3ed
e19b61f
547f356
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,7 +2,9 @@ package cloudapi | |||||
|
||||||
import ( | ||||||
"bytes" | ||||||
"encoding/json" | ||||||
"fmt" | ||||||
"io" | ||||||
"mime/multipart" | ||||||
"net/http" | ||||||
"strconv" | ||||||
|
@@ -336,3 +338,95 @@ func (c *Client) ValidateToken() (*ValidateTokenResponse, error) { | |||||
|
||||||
return &vtr, nil | ||||||
} | ||||||
|
||||||
// Organization represents a Grafana Cloud k6 organization | ||||||
type Organization struct { | ||||||
ID int `json:"id"` | ||||||
GrafanaStackName string `json:"grafana_stack_name"` | ||||||
GrafanaStackID int `json:"grafana_stack_id"` | ||||||
} | ||||||
|
||||||
// AccountMeResponse represents the response of the /account/me endpoint. | ||||||
type AccountMeResponse struct { | ||||||
Organizations []Organization `json:"organizations"` | ||||||
} | ||||||
|
||||||
// AccountMe retrieves the current user's account information. | ||||||
func (c *Client) AccountMe() (*AccountMeResponse, error) { | ||||||
// TODO: remove this hardcoded URL | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the problem with this? Can we create a dedicated client? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I agree that as we usually rely on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess that, at the very least, we should parameterize the |
||||||
req, err := c.NewRequest("GET", "https://api.k6.io/v3/account/me", nil) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Same for the others |
||||||
if err != nil { | ||||||
return nil, err | ||||||
} | ||||||
|
||||||
amr := AccountMeResponse{} | ||||||
err = c.Do(req, &amr) | ||||||
if err != nil { | ||||||
return nil, err | ||||||
} | ||||||
|
||||||
return &amr, nil | ||||||
} | ||||||
|
||||||
// GetDefaultProject retrieves the default project for the given stack ID. | ||||||
func (c *Client) GetDefaultProject(stackID int64) (int64, string, error) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know what the long-term plans are here. The new REST API does a few things differently:
Should we change the existing Client, or add a new one? What do you think about using the auto-generated Go SDK? Moving to the new API completely is impossible until we enforce StackID (maybe in v2?). So yeah, we will be living in this mixed APIs state for a while. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is it really a strict requirement? It seems we can get the StackID from the token just by calling
It sounds to me a great idea to explore. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As discussed above, I'd suggest adding a new client, so we keep things isolated, especially considering we're speaking about different hosts or different versions of the API.
I think we should use it when possible, but I suspect most of the operations that are supported now (by k6) aren't supported by the latest version of the Public API. But yeah, if we start adding support into k6 for the operations available through the auto-generated client, then I'd suggest using it for those, for sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A user can be in many Stacks! Which one should we pick? 👀 That's the current problem, IMO.
I'll try to do this. I'll also ask the backend about the That said, I'll do the smallest thing we can in this area to move this PR forward. We can keep adding things to that new client in future PRs (e.g., when I add |
||||||
// TODO: remove this hardcoded URL | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as before |
||||||
req, err := c.NewRequest("GET", "https://api.k6.io/cloud/v6/projects", nil) | ||||||
if err != nil { | ||||||
return 0, "", err | ||||||
} | ||||||
|
||||||
q := req.URL.Query() | ||||||
q.Add("$orderby", "created") | ||||||
req.URL.RawQuery = q.Encode() | ||||||
|
||||||
req.Header.Set("X-Stack-Id", fmt.Sprintf("%d", stackID)) | ||||||
// TODO: by default the client uses Token instead of Bearer | ||||||
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", c.token)) | ||||||
req.Header.Set("User-Agent", "Go-http-client") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Is it required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess Backend folks may want to know where the API is being consumed from, right @dgzlopes? 🤔 If we ever replace this custom implementation in favor of the auto-generated Go SDK, the User-Agent is yet another configuration attribute that you can customize. Wrt the value, I think we might want to use something that explicitly let us know this request is coming from the k6 binary. |
||||||
|
||||||
// TODO: Can't use c.Do bc it messes up the headers | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is yet another indicator that we should likely have a different client implementation for this. |
||||||
resp, err := http.DefaultClient.Do(req) | ||||||
if err != nil { | ||||||
Comment on lines
+388
to
+390
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then we need a dedicated client overwriting this different logic, otherwise testing will be quite difficult |
||||||
return 0, "", fmt.Errorf("request failed: %w", err) | ||||||
} | ||||||
defer func() { | ||||||
if resp != nil { | ||||||
_, _ = io.Copy(io.Discard, resp.Body) | ||||||
if cerr := resp.Body.Close(); cerr != nil && err == nil { | ||||||
err = cerr | ||||||
} | ||||||
} | ||||||
}() | ||||||
|
||||||
if resp.StatusCode != http.StatusOK { | ||||||
body, _ := io.ReadAll(resp.Body) | ||||||
return 0, "", fmt.Errorf("unexpected status %d: %s", resp.StatusCode, string(body)) | ||||||
} | ||||||
|
||||||
var parsed struct { | ||||||
Count int64 `json:"@count"` | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Do we need to parse it? |
||||||
Value []struct { | ||||||
ID int64 `json:"id"` | ||||||
Name string `json:"name"` | ||||||
IsDefault bool `json:"is_default"` | ||||||
FolderUID string `json:"grafana_folder_uid"` | ||||||
} `json:"value"` | ||||||
} | ||||||
|
||||||
if err := json.NewDecoder(resp.Body).Decode(&parsed); err != nil { | ||||||
return 0, "", fmt.Errorf("failed to decode response: %w", err) | ||||||
} | ||||||
|
||||||
if len(parsed.Value) == 0 { | ||||||
return 0, "", fmt.Errorf("no projects found for stack ID %d", stackID) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We should be consistent, if we prefer the Slug on input then we should return a slug on errors. Otherwise, they will experience the same difficulties we're carefully avoiding on inputs. |
||||||
} | ||||||
|
||||||
for _, proj := range parsed.Value { | ||||||
if proj.IsDefault { | ||||||
return proj.ID, proj.Name, nil | ||||||
} | ||||||
} | ||||||
|
||||||
return 0, "", fmt.Errorf("no default project found for stack ID %d", stackID) | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -18,6 +18,8 @@ const LegacyCloudConfigKey = "loadimpact" | |||||
//nolint:lll | ||||||
type Config struct { | ||||||
// TODO: refactor common stuff between cloud execution and output | ||||||
StackID null.Int `json:"stackID,omitempty" envconfig:"K6_CLOUD_STACK_ID"` | ||||||
StackSlug null.String `json:"stackSlug,omitempty" envconfig:"K6_CLOUD_STACK_SLUG"` | ||||||
Token null.String `json:"token" envconfig:"K6_CLOUD_TOKEN"` | ||||||
ProjectID null.Int `json:"projectID" envconfig:"K6_CLOUD_PROJECT_ID"` | ||||||
Name null.String `json:"name" envconfig:"K6_CLOUD_NAME"` | ||||||
|
@@ -103,6 +105,12 @@ func NewConfig() Config { | |||||
// | ||||||
//nolint:cyclop | ||||||
func (c Config) Apply(cfg Config) Config { | ||||||
if cfg.StackID.Valid { | ||||||
c.StackID = cfg.StackID | ||||||
} | ||||||
if cfg.StackSlug.Valid { | ||||||
c.StackSlug = cfg.StackSlug | ||||||
} | ||||||
if cfg.Token.Valid { | ||||||
c.Token = cfg.Token | ||||||
} | ||||||
|
@@ -240,7 +248,8 @@ func mergeFromCloudOptionAndExternal( | |||||
if err := json.Unmarshal(source, &tmpConfig); err != nil { | ||||||
return err | ||||||
} | ||||||
// Only take out the ProjectID, Name and Token from the options.cloud (or legacy loadimpact struct) map: | ||||||
|
||||||
// Only take out the ProjectID, Name, Token and StackID from the options.cloud (or legacy loadimpact struct) map: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
if tmpConfig.ProjectID.Valid { | ||||||
conf.ProjectID = tmpConfig.ProjectID | ||||||
} | ||||||
|
@@ -250,6 +259,12 @@ func mergeFromCloudOptionAndExternal( | |||||
if tmpConfig.Token.Valid { | ||||||
conf.Token = tmpConfig.Token | ||||||
} | ||||||
if tmpConfig.StackID.Valid { | ||||||
conf.StackID = tmpConfig.StackID | ||||||
} | ||||||
if tmpConfig.StackSlug.Valid { | ||||||
conf.StackSlug = tmpConfig.StackSlug | ||||||
} | ||||||
|
||||||
return nil | ||||||
} | ||||||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
I have tested the endpoint, and looks like it could return a null
grafana_stack_name
and/orgrafana_stack_id
.Is that only a particular case for my account? Or an edge case that we should handle?
Because, in such case, I think we should make this
nil
-able, and/or somehow explicitly document that it can be the case.I'm wondering the wide variety of corner cases we can find once we release this to users 🤔