James C CLI updates tags #490
Conversation
…s we will need to make another UpdateRunRequest but purely for tags Signed-off-by: James Cocker <[email protected]>
…added command name into commandCollection Signed-off-by: James Cocker <[email protected]>
…fficiency addittions Signed-off-by: James Cocker <[email protected]>
…only shown if log flag used Signed-off-by: James Cocker <[email protected]>
Signed-off-by: James Cocker <[email protected]>
…rent dir Signed-off-by: James Cocker <[email protected]>
| * [galasactl runs prepare](galasactl_runs_prepare.md) - prepares a list of tests | ||
| * [galasactl runs reset](galasactl_runs_reset.md) - reset an active run in the ecosystem | ||
| * [galasactl runs submit](galasactl_runs_submit.md) - submit a list of tests to the ecosystem | ||
| * [galasactl runs update](galasactl_runs_update.md) - Update tags for a named test run. |
There was a problem hiding this comment.
While it's true that runs update only updates tags at the moment, maybe this description could be made more general to something like: "Update the record of an existing test run on a Galasa service"?
It's also worth explicitly saying that this affects existing test runs on a Galasa service since I don't believe we're setting tags for local runs.
| ### Options | ||
|
|
||
| ``` | ||
| --add-tags strings comma-separated list of tags to add |
There was a problem hiding this comment.
Can the --add-tags flag also be supplied like:
--add-tags tag1 --add-tags tag2
If so, it would be good to make it clear that you can either supply a comma-separated list or multiple --add-tags flags
| --add-tags strings comma-separated list of tags to add | ||
| -h, --help Displays the options for the 'runs update' command. | ||
| --name string the name of the test run we want to update | ||
| --remove-tags strings comma-separated list of tags to remove |
There was a problem hiding this comment.
Can the --remove-tags flag also be supplied like:
--remove-tags tag1 --remove-tags tag2
If so, it would be good to make it clear that you can either supply a comma-separated list or multiple --remove-tags flags
| func createUpdateRunRequest(status string, result string) *galasaapi.UpdateRunRequest { | ||
| var UpdateRunRequest = galasaapi.NewUpdateRunRequest() | ||
| func createUpdateRunStatusRequest(status string, result string) *galasaapi.UpdateRunRequest { | ||
| var UpdateRunStatusRequest = galasaapi.NewUpdateRunRequest() |
There was a problem hiding this comment.
Very minor, can the UpdateRunStatusRequest variable use camel case (i.e. remove the capital U at the start :))
|
|
||
| UpdateRunRequest := createUpdateRunRequest(CANCEL_STATUS, CANCEL_RESULT) | ||
| err = cancelRun(runName, runId, UpdateRunRequest, commsClient) | ||
| UpdateRunStatusRequest := createUpdateRunStatusRequest(CANCEL_STATUS, CANCEL_RESULT) |
There was a problem hiding this comment.
Same here, the UpdateRunStatusRequest variable should be in camel case
| err = updateRuns(runs, addTags, removeTags, commsClient) | ||
| } | ||
| } else { | ||
| console.WriteString(err.Error()) |
There was a problem hiding this comment.
Wondering if we need this - if the error is returned, wouldn't the CLI output the error?
If that's the case, then this might cause the same error to be printed out twice
| if err != nil { | ||
| if httpResponse == nil { | ||
| // We never got a response, error sending it or something ? | ||
| err = galasaErrors.NewGalasaError(galasaErrors.GALASA_ERROR_RESET_RUN_FAILED, runName, err.Error()) |
There was a problem hiding this comment.
Looks like the wrong error message is being used here
| } else { | ||
| statusCode := httpResponse.StatusCode | ||
| if statusCode != http.StatusAccepted && statusCode != http.StatusOK { | ||
| err = galasaErrors.NewGalasaError(galasaErrors.GALASA_ERROR_RESET_RUN_RESPONSE_PARSING) |
There was a problem hiding this comment.
Looks like the wrong error message is being used here as well
| log.Println("Validating the provided tag name") | ||
|
|
||
| err = validateStringIsLatin1AndNotBlank(tagName, galasaErrors.GALASA_ERROR_INVALID_TAG_NAME) | ||
| err = validateStringIsLatin1AndNotBlank(tagName, galasaErrors.GALASA_ERROR_DELETE_TAG_INVALID_NAME) |
There was a problem hiding this comment.
This error is being used in galasactl tags set, but the error now says ERROR_DELETE_TAG. Maybe keep the error as it was previously?
| if runName == "" { | ||
| return galasaErrors.NewGalasaError(galasaErrors.GALASA_ERROR_MISSING_NAME_FLAG, "--name") | ||
| } | ||
|
|
||
| if len(addTags) == 0 && len(removeTags) == 0 { | ||
| err = galasaErrors.NewGalasaError(galasaErrors.GALASA_ERROR_UPDATE_RUN_MISSING_FIELD, "--add-tags or --remove-tags") | ||
| return err | ||
| } | ||
|
|
||
| // Validate the runName as best we can without contacting the ecosystem. | ||
| err = ValidateRunName(runName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| addTags = removeDuplicateTags(addTags) | ||
| removeTags = removeDuplicateTags(removeTags) | ||
|
|
||
| // Check for tags that are both added and removed. | ||
| for _, tag := range addTags { | ||
| if (slices.Contains(removeTags, tag)) { | ||
| return galasaErrors.NewGalasaError(galasaErrors.GALASA_ERROR_UPDATE_RUN_INVALID_TAG_UPDATE, tag) | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it be worth moving this validation into its own function and then calling the validation function here?
Why?
Solves #2465 by adding a new command
galasactl runs updatewhich can be used like the following to alter tags on an existing rungalasactl runs update --name U12345 --add-tags tag1,tag2 --remove-tags tag3.