Introducing Workspaces into pipelines #793
Conversation
bhanurp
commented
Jul 2, 2023
•
bhanurp
commented
- All tests passed. If this feature is not already covered by the tests, I added new tests.
- All static analysis checks passed.
- This pull request is on the dev branch.
- I used gofmt for formatting the code before submitting the pull request.
This reverts commit 3ac03a5.
yahavi
added
new feature
github-actions
bot
removed
the
safe to test
There was a problem hiding this comment.
@bhanurp
Good work!
I reviewed your code and added some comments. I think a second round of code review is needed after fixing the issues.
Important -
- Ensure the tests are not failing
- Add all new methods in manager.go to the README
| workspacesResponseStatus, err := sm.WorkspacePollSyncStatus() | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Can this login be inside workspaceService.GetWorkspacePipelines? Let's try to keep the manager clean.
| } | ||
|
|
||
| func TestWorkspaceValidationWhenPipelinesResourcesAreNotValid(t *testing.T) { | ||
|
|
There was a problem hiding this comment.
Please remove all "if"s from the tests. Use only "assert" or "require".
This file needs to be reviewed again after this change.
tests/testdata/pip.yaml
Outdated
There was a problem hiding this comment.
Let's avoid using the word "pip" for JFrog pipelines in this and other JFrog CLI components. You can name this file "pipe".
| ) | ||
|
|
||
| func TestPipelinesWorkspaceService(t *testing.T) { | ||
| t.Run("test trigger pipelines workspace when resources are not valid", TestWorkspaceValidationWhenPipelinesResourcesAreNotValid) |
There was a problem hiding this comment.
| t.Run("test trigger pipelines workspace when resources are not valid", TestWorkspaceValidationWhenPipelinesResourcesAreNotValid) | |
| initPipelinesTest(t) | |
| t.Run("test trigger pipelines workspace when resources are not valid", TestWorkspaceValidationWhenPipelinesResourcesAreNotValid) |
| httpDetails := vs.getHttpDetails() | ||
|
|
||
| // Query params | ||
| m := make(map[string]string, 0) |
| if nil != resp && resp.StatusCode == http.StatusOK { | ||
| log.Info("Processing resources yaml response ") | ||
| log.Info(string(body)) | ||
| err = processValidatePipResourceResponse(body) | ||
| if err != nil { | ||
| log.Error("Failed to process resources validation response") | ||
| } | ||
| } |
There was a problem hiding this comment.
You already check the status code in CheckResponseStatusWithBody. Please also consider nil before "CheckResponseStatusWithBody".
| func (vs *ValidateService) constructValidateAPIURL(qParams map[string]string, apiPath string) string { | ||
| uri, err := url.Parse(vs.ServiceDetails.GetUrl() + apiPath) | ||
| if err != nil { | ||
| log.Error("Failed to parse pipelines fetch run status url") |
Co-authored-by: Yahav Itzhak <[email protected]>
|
Closing this PR for now. |
