-
Notifications
You must be signed in to change notification settings - Fork 13
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
State revert uses the build planner #2858
Changes from 5 commits
c0ad8fa
e7b1401
69aa7d1
824ecad
b251994
f6fb72d
1611afe
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 |
---|---|---|
|
@@ -11,7 +11,6 @@ import ( | |
"github.com/ActiveState/cli/internal/runbits/commitmediator" | ||
"github.com/ActiveState/cli/pkg/cmdlets/commit" | ||
gqlmodel "github.com/ActiveState/cli/pkg/platform/api/graphql/model" | ||
"github.com/ActiveState/cli/pkg/platform/api/mono/mono_models" | ||
"github.com/ActiveState/cli/pkg/platform/authentication" | ||
"github.com/ActiveState/cli/pkg/platform/model" | ||
"github.com/ActiveState/cli/pkg/platform/runtime/target" | ||
|
@@ -65,39 +64,33 @@ func (r *Revert) Run(params *Params) error { | |
if err != nil { | ||
return errs.Wrap(err, "Unable to get local commit") | ||
} | ||
|
||
if params.CommitID == latestCommit.String() && params.To { | ||
return locale.NewInputError("err_revert_to_current_commit", "The commit to revert to cannot be the latest commit") | ||
} | ||
r.out.Notice(locale.Tl("operating_message", "", r.project.NamespaceString(), r.project.Dir())) | ||
commitID := strfmt.UUID(params.CommitID) | ||
|
||
var targetCommit *mono_models.Commit // the commit to revert the contents of, or the commit to revert to | ||
var fromCommit, toCommit strfmt.UUID | ||
if !params.To { | ||
priorCommits, err := model.CommitHistoryPaged(commitID, 0, 2) | ||
if err != nil { | ||
return errs.AddTips( | ||
locale.WrapError(err, "err_revert_get_commit", "", params.CommitID), | ||
locale.T("tip_private_project_auth"), | ||
) | ||
} | ||
if priorCommits.TotalCommits < 2 { | ||
return locale.NewInputError("err_revert_no_history", "Cannot revert commit {{.V0}}: no prior history", params.CommitID) | ||
} | ||
targetCommit = priorCommits.Commits[0] | ||
fromCommit = commitID | ||
toCommit = priorCommits.Commits[1].CommitID // parent commit | ||
} else { | ||
var err error | ||
targetCommit, err = model.GetCommitWithinCommitHistory(latestCommit, commitID) | ||
if err != nil { | ||
return errs.AddTips( | ||
locale.WrapError(err, "err_revert_get_commit", "", params.CommitID), | ||
locale.T("tip_private_project_auth"), | ||
) | ||
} | ||
fromCommit = latestCommit | ||
toCommit = targetCommit.CommitID | ||
bp := model.NewBuildPlannerModel(r.auth) | ||
targetCommitID := params.CommitID // the commit to revert the contents of, or the commit to revert to | ||
revertParams := revertParams{ | ||
organization: r.project.Owner(), | ||
project: r.project.Name(), | ||
parentCommitID: latestCommit.String(), | ||
revertCommitID: params.CommitID, | ||
} | ||
revertFunc := r.revertCommit | ||
preposition := "" | ||
if params.To { | ||
revertFunc = r.revertToCommit | ||
preposition = " to" // need leading whitespace | ||
} | ||
|
||
targetCommit, err := model.GetCommitWithinCommitHistory(latestCommit, strfmt.UUID(targetCommitID)) | ||
if err != nil { | ||
return errs.AddTips( | ||
locale.WrapError(err, "err_revert_get_commit", "", params.CommitID), | ||
locale.T("tip_private_project_auth"), | ||
) | ||
} | ||
|
||
var orgs []gqlmodel.Organization | ||
|
@@ -108,10 +101,7 @@ func (r *Revert) Run(params *Params) error { | |
return locale.WrapError(err, "err_revert_get_organizations", "Could not get organizations for current user") | ||
} | ||
} | ||
preposition := "" | ||
if params.To { | ||
preposition = " to" // need leading whitespace | ||
} | ||
|
||
if !r.out.Type().IsStructured() { | ||
r.out.Print(locale.Tl("revert_info", "You are about to revert{{.V0}} the following commit:", preposition)) | ||
commit.PrintCommit(r.out, targetCommit, orgs) | ||
|
@@ -126,41 +116,72 @@ func (r *Revert) Run(params *Params) error { | |
return locale.NewInputError("err_revert_aborted", "Revert aborted by user") | ||
} | ||
|
||
revertCommit, err := model.RevertCommitWithinHistory(fromCommit, toCommit, latestCommit) | ||
revertCommit, err := revertFunc(revertParams, bp) | ||
if err != nil { | ||
return errs.AddTips( | ||
locale.WrapError(err, "err_revert_commit", "", preposition, params.CommitID), | ||
locale.Tl("tip_revert_sync", "Please ensure that the local project is synchronized with the platform and that the given commit ID belongs to the current project"), | ||
locale.T("tip_private_project_auth")) | ||
} | ||
|
||
err = runbits.RefreshRuntime(r.auth, r.out, r.analytics, r.project, revertCommit.CommitID, true, target.TriggerRevert, r.svcModel) | ||
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") | ||
Comment on lines
+127
to
+134
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. Good call to swap these two. |
||
} | ||
|
||
r.out.Print(output.Prepare( | ||
locale.Tl("revert_success", "Successfully reverted{{.V0}} commit: {{.V1}}", preposition, params.CommitID), | ||
&struct { | ||
CurrentCommitID string `json:"current_commit_id"` | ||
}{ | ||
revertCommit.CommitID.String(), | ||
revertCommit.String(), | ||
}, | ||
)) | ||
r.out.Notice(locale.T("operation_success_local")) | ||
return nil | ||
} | ||
|
||
func containsCommitID(history []*mono_models.Commit, commitID strfmt.UUID) bool { | ||
for _, c := range history { | ||
if c.CommitID == commitID { | ||
return true | ||
} | ||
type revertFunc func(params revertParams, bp *model.BuildPlanner) (strfmt.UUID, error) | ||
|
||
type revertParams struct { | ||
organization string | ||
project string | ||
parentCommitID string | ||
revertCommitID string | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: capitalize "Could" |
||
} | ||
|
||
return newCommitID, nil | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: capitalize "Could" |
||
} | ||
|
||
stageCommitParams := model.StageCommitParams{ | ||
Owner: params.organization, | ||
Project: params.project, | ||
ParentCommit: params.parentCommitID, | ||
Description: locale.Tl("revert_commit_description", "Revert to commit {{.V0}}", params.revertCommitID), | ||
Expression: buildExpression, | ||
} | ||
return false | ||
|
||
newCommitID, err := bp.StageCommit(stageCommitParams) | ||
if err != nil { | ||
return "", errs.Wrap(err, "could not stage commit") | ||
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. Nit: capitalize "Could" |
||
} | ||
|
||
return newCommitID, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,22 +46,29 @@ const ( | |
BuildLogRecipeID = "RECIPE_ID" | ||
BuildRequestID = "BUILD_REQUEST_ID" | ||
|
||
// Version Comparators | ||
ComparatorEQ string = "eq" | ||
ComparatorGT = "gt" | ||
ComparatorGTE = "gte" | ||
ComparatorLT = "lt" | ||
ComparatorLTE = "lte" | ||
ComparatorNE = "ne" | ||
|
||
// Version Requirement keys | ||
VersionRequirementComparatorKey = "comparator" | ||
VersionRequirementVersionKey = "version" | ||
|
||
// MIME types | ||
XArtifactMimeType = "application/x.artifact" | ||
XActiveStateArtifactMimeType = "application/x-activestate-artifacts" | ||
XCamelInstallerMimeType = "application/x-camel-installer" | ||
XGozipInstallerMimeType = "application/x-gozip-installer" | ||
XActiveStateBuilderMimeType = "application/x-activestate-builder" | ||
|
||
// RevertCommit strategies | ||
RevertCommitStrategyForce = "Force" | ||
RevertCommitStrategyDefault = "Default" | ||
|
||
// MergeCommit strategies | ||
MergeCommitStrategyRecursive MergeStrategy = "Recursive" | ||
MergeCommitStrategyRecursiveOverwriteOnConflict MergeStrategy = "RecursiveOverwriteOnConflict" | ||
|
@@ -408,6 +415,18 @@ type CreateProjectResult struct { | |
ProjectCreated *projectCreated `json:"createProject"` | ||
} | ||
|
||
type RevertedCommit struct { | ||
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. Does this type need to be public? The 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. Nope, it can be private |
||
Type string `json:"__typename"` | ||
Commit *Commit `json:"commit"` | ||
CommonAncestor strfmt.UUID `json:"commonAncestorID"` | ||
ConflictPaths []string `json:"conflictPaths"` | ||
*Error | ||
} | ||
|
||
type RevertCommitResult struct { | ||
RevertedCommit *RevertedCommit `json:"revertCommit"` | ||
} | ||
|
||
type mergedCommit struct { | ||
Type string `json:"__typename"` | ||
Commit *Commit `json:"commit"` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
package request | ||
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. Hey, I understand this now! |
||
|
||
import "github.com/ActiveState/cli/pkg/platform/api/buildplanner/model" | ||
|
||
func RevertCommit(organization, project, targetVcsRef, commitID string) *revertCommit { | ||
return &revertCommit{map[string]interface{}{ | ||
"organization": organization, | ||
"project": project, | ||
"targetVcsRef": targetVcsRef, | ||
"commitId": commitID, | ||
// Currently, we use the force strategy for all revert commits. | ||
// This is because we don't have a way to show the user the conflicts | ||
// and let them resolve them yet. | ||
// https://activestatef.atlassian.net/browse/AR-80?focusedCommentId=46998 | ||
"strategy": model.RevertCommitStrategyForce, | ||
}} | ||
} | ||
|
||
type revertCommit struct { | ||
vars map[string]interface{} | ||
} | ||
|
||
func (r *revertCommit) Query() string { | ||
return ` | ||
mutation ($organization: String!, $project: String!, $commitId: String!, $targetVcsRef: String!, $strategy: RevertStrategy) { | ||
revertCommit( | ||
input: {organization: $organization, project: $project, commitId: $commitId, targetVcsRef: $targetVcsRef, strategy: $strategy} | ||
) { | ||
... on RevertedCommit { | ||
__typename | ||
commit { | ||
__typename | ||
commitId | ||
} | ||
} | ||
... on RevertConflict { | ||
__typename | ||
message | ||
commitId | ||
targetCommitId | ||
conflictPaths | ||
} | ||
... on CommitHasNoParent { | ||
__typename | ||
message | ||
} | ||
... on NotFound { | ||
__typename | ||
message | ||
mayNeedAuthentication | ||
} | ||
... on ParseError { | ||
__typename | ||
message | ||
path | ||
} | ||
... on ValidationError { | ||
__typename | ||
message | ||
} | ||
... on Forbidden { | ||
__typename | ||
message | ||
} | ||
... on HeadOnBranchMoved { | ||
__typename | ||
message | ||
} | ||
... on NoChangeSinceLastCommit { | ||
message | ||
commitId | ||
} | ||
} | ||
}` | ||
} | ||
|
||
func (r *revertCommit) Vars() map[string]interface{} { | ||
return r.vars | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -395,6 +395,29 @@ func (bp *BuildPlanner) CreateProject(params *CreateProjectParams) (strfmt.UUID, | |
return resp.ProjectCreated.Commit.CommitID, nil | ||
} | ||
|
||
func (bp *BuildPlanner) RevertCommit(organization, project, parentCommitID, commitID string) (strfmt.UUID, error) { | ||
logging.Debug("RevertCommit, organization: %s, project: %s, commitID: %s", organization, project, commitID) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: capitalize "Failed"? |
||
} | ||
|
||
if resp.RevertedCommit == nil { | ||
return "", errs.New("Commit is nil") | ||
} | ||
|
||
if bpModel.IsErrorResponse(resp.RevertedCommit.Type) { | ||
return "", bpModel.ProcessCommitError(resp.RevertedCommit.Commit, "Could not revert commit") | ||
} | ||
|
||
if resp.RevertedCommit.Commit.CommitID == "" { | ||
return "", errs.New("Commit does not contain commitID") | ||
} | ||
|
||
return resp.RevertedCommit.Commit.CommitID, nil | ||
} | ||
|
||
type MergeCommitParams struct { | ||
Owner string | ||
Project string | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,7 +42,7 @@ func (suite *RevertIntegrationTestSuite) TestRevert() { | |
e2e.OptArgs("history"), | ||
e2e.OptWD(wd), | ||
) | ||
cp.Expect("Revert commit " + commitID) | ||
cp.Expect("Reverted commit for commit " + commitID) | ||
cp.Expect("- urllib3") | ||
cp.Expect("+ argparse") // parent commit | ||
cp.Expect("+ urllib3") // commit whose changes were just reverted | ||
|
@@ -83,7 +83,7 @@ func (suite *RevertIntegrationTestSuite) TestRevert_failsOnCommitNotInHistory() | |
cp.Expect(fmt.Sprintf("Operating on project %s", namespace)) | ||
cp.SendLine("Y") | ||
cp.Expect(commitID) | ||
cp.Expect("The commit being reverted is not within the current commit's history") | ||
cp.Expect("The target commit is not within the current commit's history") | ||
cp.ExpectNotExitCode(0) | ||
} | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is printed to the user in this case? Should we be asserting it? 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. Good catch, this is actually the commit message that is now different with the buildplanner |
||
cp.Expect("- argparse") // effectively reverting previous commit | ||
cp.Expect("+ argparse") // commit being effectively reverted | ||
cp.Expect("+ urllib3") // commit reverted to | ||
|
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 lack of space was intentional so that we could have
Could not revert commit: X
orCould not revert to commit: X
(see https://github.com/ActiveState/cli/pull/2858/files#diff-b6b503d1d2e8bd95d174c9cf7c989b67e26bb91a2882e96cb6a916cafc55fbc1R85). Now it could beCould not revert commit: X
(note the two spaces). Unless I'm missing something, please revert this change.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.
Ahh okay, thanks for catching that!