-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add support to configure the default stack used by Cloud commands #5420
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 5 commits
9c73b81
c91b1b0
1c0d291
b91bf75
bf3e61d
4d8aa50
ee18a7e
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" | ||||||||
|
|
@@ -70,6 +72,12 @@ type ValidateTokenResponse struct { | |||||||
| Token string `json:"token-info"` | ||||||||
| } | ||||||||
|
|
||||||||
| // AuthenticationResponse represents the response from the /cloud/v6/auth endpoint. | ||||||||
| type AuthenticationResponse struct { | ||||||||
| StackID int64 `json:"stack_id"` | ||||||||
| DefaultProjectID int64 `json:"default_project_id"` | ||||||||
| } | ||||||||
|
|
||||||||
| func (c *Client) handleLogEntriesFromCloud(ctrr CreateTestRunResponse) { | ||||||||
| logger := c.logger.WithField("source", "grafana-k6-cloud") | ||||||||
| for _, logEntry := range ctrr.Logs { | ||||||||
|
|
@@ -336,3 +344,40 @@ func (c *Client) ValidateToken() (*ValidateTokenResponse, error) { | |||||||
|
|
||||||||
| return &vtr, nil | ||||||||
| } | ||||||||
|
|
||||||||
| // ValidateAuth validates a token and stack URL, returning the stack ID and default project ID. | ||||||||
| // The stackURL must be a normalized full URL (e.g., https://my-team.grafana.net). | ||||||||
| func (c *Client) ValidateAuth(stackURL string) (*AuthenticationResponse, 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. Can we use the generated go client for this operation?
Contributor
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 have the same question actually. And more than that, should it be the goal for all v6 endpoints to be called via that library?
Contributor
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 it would be great but would probably need a larger refactor, not directly related to what this PR does. I'm planning to use this library when working on #5009 and I can update this function then. |
||||||||
| req, err := http.NewRequest(http.MethodGet, "https://api.k6.io/cloud/v6/auth", nil) //nolint:noctx | ||||||||
|
||||||||
| req, err := http.NewRequest(http.MethodGet, "https://api.k6.io/cloud/v6/auth", nil) //nolint:noctx | |
| req, err := http.NewRequestWithContext(http.MethodGet, c.authBase, nil) //nolint:noctx |
In addition, we should use the context, so we can cancel the requests when the user aborts the script.
Outdated
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.
Probably not blocking nor critical 👇🏻
I understand why this cannot use c.baseURL here, as the baseURL contains the v1 path. However, I would prefer for us not to hardcode the API URL here if possible.
I would suggest refactoring the client if possible to store the API version it talks to, and select the base api path to use according:
- "/v1" if talking to the v1 API
- "/cloud/v6" if talking to the v6 API
Specifically thinking of this code here:
Line 44 in cefb446
| baseURL: fmt.Sprintf("%s/v1", host), |
In an ideal world, I think this should call something along the lines of c.BaseURL(cloudAPIVersion6) and get a fully qualified url as a result 🙇🏻
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 made the changes! Let me know what you think 👀
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.
The error schema in v6 AP is different from the old one. Should the error handling code be updated too?
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,9 +18,12 @@ const LegacyCloudConfigKey = "loadimpact" | |||||
| //nolint:lll | ||||||
| type Config struct { | ||||||
| // TODO: refactor common stuff between cloud execution and output | ||||||
| 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"` | ||||||
| StackID null.Int `json:"stackID,omitempty" envconfig:"K6_CLOUD_STACK_ID"` | ||||||
| StackURL null.String `json:"stackURL,omitempty" envconfig:"K6_CLOUD_STACK"` | ||||||
|
Contributor
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
I added URL because 2 out of 3 have it, but if the intention is to not have it then remove it from the other two. |
||||||
| DefaultProjectID null.Int `json:"defaultProjectID,omitempty"` | ||||||
| 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"` | ||||||
|
|
||||||
| Host null.String `json:"host" envconfig:"K6_CLOUD_HOST"` | ||||||
| Timeout types.NullDuration `json:"timeout" envconfig:"K6_CLOUD_TIMEOUT"` | ||||||
|
|
@@ -103,6 +106,15 @@ func NewConfig() Config { | |||||
| // | ||||||
| //nolint:cyclop | ||||||
| func (c Config) Apply(cfg Config) Config { | ||||||
| if cfg.StackID.Valid { | ||||||
| c.StackID = cfg.StackID | ||||||
| } | ||||||
| if cfg.StackURL.Valid && !c.StackURL.Valid { | ||||||
| c.StackURL = cfg.StackURL | ||||||
| } | ||||||
| if cfg.DefaultProjectID.Valid { | ||||||
| c.DefaultProjectID = cfg.DefaultProjectID | ||||||
| } | ||||||
| if cfg.Token.Valid { | ||||||
| c.Token = cfg.Token | ||||||
| } | ||||||
|
|
@@ -240,7 +252,9 @@ 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 merge ProjectID, Name, Token, and StackID from options. | ||||||
| // StackURL and DefaultProjectID can only be set via login. | ||||||
|
Comment on lines
+255
to
+257
Contributor
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. Can we get this check in a position near to the others, please? |
||||||
| if tmpConfig.ProjectID.Valid { | ||||||
| conf.ProjectID = tmpConfig.ProjectID | ||||||
| } | ||||||
|
|
@@ -250,6 +264,9 @@ func mergeFromCloudOptionAndExternal( | |||||
| if tmpConfig.Token.Valid { | ||||||
| conf.Token = tmpConfig.Token | ||||||
| } | ||||||
| if tmpConfig.StackID.Valid { | ||||||
| conf.StackID = tmpConfig.StackID | ||||||
| } | ||||||
|
|
||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
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.
do we really need to export it?