State revert uses the build planner #2858
Conversation
MDrakos
commented
Oct 30, 2023
•
MDrakos
commented
 DX-2226 `state revert --to` uses the BuildPlanner
|
internal/locale/locales/en-us.yaml
Outdated
| @@ -1703,7 +1703,7 @@ err_user_network_solution: | |||
| err_revert_get_commit: | |||
| other: "Could not fetch commit details for commit with ID: {{.V0}}" | |||
| err_revert_commit: | |||
| other: "Could not revert{{.V0}} commit: {{.V1}}" | |||
| other: "Could not revert {{.V0}} commit: {{.V1}}" | |||
There was a problem hiding this comment.
The lack of space was intentional so that we could have Could not revert commit: X or Could not revert to commit: X (see https://github.com/ActiveState/cli/pull/2858/files#diff-b6b503d1d2e8bd95d174c9cf7c989b67e26bb91a2882e96cb6a916cafc55fbc1R85). Now it could be Could not revert commit: X (note the two spaces). Unless I'm missing something, please revert this change.
There was a problem hiding this comment.
Ahh okay, thanks for catching that!
internal/runners/revert/revert.go
Outdated
| func (r *Revert) revertCommit(params revertParams, bp *model.BuildPlanner) (strfmt.UUID, error) { | ||
| newCommitID, err := bp.RevertCommit(params.organization, params.project, params.parentCommitID, params.revertCommitID) | ||
| if err != nil { | ||
| return "", errs.Wrap(err, "could not revert commit") |
There was a problem hiding this comment.
Nit: capitalize "Could"
internal/runners/revert/revert.go
Outdated
| func (r *Revert) revertToCommit(params revertParams, bp *model.BuildPlanner) (strfmt.UUID, error) { | ||
| buildExpression, err := bp.GetBuildExpression(params.organization, params.project, params.revertCommitID) | ||
| if err != nil { | ||
| return "", errs.Wrap(err, "could not get build expression") |
There was a problem hiding this comment.
Nit: capitalize "Could"
| @@ -0,0 +1,79 @@ | |||
| package request | |||
There was a problem hiding this comment.
Hey, I understand this now!
pkg/platform/model/buildplanner.go
Outdated
| resp := &bpModel.RevertCommitResult{} | ||
| err := bp.client.Run(request.RevertCommit(organization, project, parentCommitID, commitID), resp) | ||
| if err != nil { | ||
| return "", processBuildPlannerError(err, "failed to revert commit") |
There was a problem hiding this comment.
Nit: capitalize "Failed"?
| @@ -115,7 +115,6 @@ func (suite *RevertIntegrationTestSuite) TestRevertTo() { | |||
| e2e.OptArgs("history"), | |||
| e2e.OptWD(wd), | |||
| ) | |||
| cp.Expect("Reverting to commit " + commitID) | |||
There was a problem hiding this comment.
What is printed to the user in this case? Should we be asserting it?
There was a problem hiding this comment.
Good catch, this is actually the commit message that is now different with the buildplanner
| err = commitmediator.Set(r.project, revertCommit.String()) | ||
| if err != nil { | ||
| return locale.WrapError(err, "err_refresh_runtime") | ||
| return errs.Wrap(err, "Unable to set local commit") | ||
| } | ||
|
|
||
| err = commitmediator.Set(r.project, revertCommit.CommitID.String()) | ||
| err = runbits.RefreshRuntime(r.auth, r.out, r.analytics, r.project, revertCommit, true, target.TriggerRevert, r.svcModel) | ||
| if err != nil { | ||
| return errs.Wrap(err, "Unable to set local commit") | ||
| return locale.WrapError(err, "err_refresh_runtime") |
There was a problem hiding this comment.
Good call to swap these two.
internal/runners/revert/revert.go
Outdated
|
|
||
| newCommitID, err := bp.StageCommit(stageCommitParams) | ||
| if err != nil { | ||
| return "", errs.Wrap(err, "could not stage commit") |
There was a problem hiding this comment.
Nit: capitalize "Could"
| @@ -408,6 +415,18 @@ type CreateProjectResult struct { | |||
| ProjectCreated *projectCreated `json:"createProject"` | |||
| } | |||
|
|
|||
| type RevertedCommit struct { | |||
There was a problem hiding this comment.
Does this type need to be public? The mergedCommit type below on line 430 is similar in function and private.
Capitalize error messages Add back expect line
test/integration/revert_int_test.go
Outdated
| cp.Expect("- argparse") // effectively reverting previous commit | ||
| cp.Expect("+ argparse") // commit being effectively reverted | ||
| cp.Expect("+ urllib3") // commit reverted to | ||
| cp.Expect("+ python") // initial commit | ||
| fmt.Println("Output: ", cp.Snapshot()) |
There was a problem hiding this comment.
I think this was accidentally left here instead of removed.
There was a problem hiding this comment.
Whoops, thanks for catching that!
