Skip to content
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

Temporarily disable localcommit files and buildscripts. #2853

Merged
merged 10 commits into from
Nov 2, 2023

Conversation

mitchell-as
Copy link
Contributor

@mitchell-as mitchell-as commented Oct 30, 2023

StoryDX-2303 Guard against project migration and local order file creation

@mitchell-as
Copy link
Contributor Author

This PR looks bigger than it is because it depends on some branches that have not yet been merged. The relevant changes are in the last two commits, which can be viewed separately here: https://github.com/ActiveState/cli/pull/2853/files/fb9374a399b28367b66094d10ecc501c97d19574..c2215ab66ec5f96f426c5f4e87e6c574c5f51396

@mitchell-as mitchell-as requested a review from MDrakos October 30, 2023 21:29
@mitchell-as mitchell-as marked this pull request as ready for review October 30, 2023 21:29
Copy link
Member

@MDrakos MDrakos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This largely looks good to me. Tagging all of the disabled stuff with the story ID will help when it comes time to reenable this or iterate on it.

"github.com/ActiveState/cli/internal/runbits/legacy/projectmigration"
"github.com/ActiveState/cli/pkg/localcommit"
// Re-enable in DX-2307.
//"github.com/ActiveState/cli/internal/errs"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be okay to let goimports handle this kind of stuff. I'm not sure how it will handle import statements with comments in them as we iterate on these files before we re-enable everything.

}
}
// Re-enable in DX-2307.
//if emptyDir || fileutils.DirExists(filepath.Join(path, ".git")) {B
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//if emptyDir || fileutils.DirExists(filepath.Join(path, ".git")) {B
//if emptyDir || fileutils.DirExists(filepath.Join(path, ".git")) {

@@ -939,6 +963,11 @@ func createCustom(params *CreateParams, lang language.Language) (*Project, error
q.Set("branch", params.BranchName)
}

// Remove this block in DX-2307.
if params.LegacyCommitID != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an error condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. In particular when running state init, this will be empty. After the project is created on the Platform, the commitID is polled and filled in retroactively.

suite.Require().True(fileutils.FileExists(commitIdFile), "ActiveState-CLI/Python3 was not checked out properly")
suite.Assert().Equal(string(fileutils.ReadFileUnsafe(commitIdFile)), "6d9280e7-75eb-401a-9e71-0d99759fbad3", "did not check out specific commit ID")
// Re-enable the following lines in DX-2307.
//suite.Require().True(fileutils.DirExists(python3Dir), "state checkout should have created "+python3Dir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should still be valid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this!

@mitchell-as
Copy link
Contributor Author

I don't know if the merge from v42 into this PR affects this review from your side, but just in case, here are the changes from your last review: https://github.com/ActiveState/cli/pull/2853/files/c2215ab66ec5f96f426c5f4e87e6c574c5f51396..eae791b1feb6c6b02fa1dd021a08d9aab73bf82e

@mitchell-as mitchell-as requested a review from MDrakos October 31, 2023 22:19
@MDrakos
Copy link
Member

MDrakos commented Nov 1, 2023

I don't know if the merge from v42 into this PR affects this review from your side, but just in case, here are the changes from your last review: https://github.com/ActiveState/cli/pull/2853/files/c2215ab66ec5f96f426c5f4e87e6c574c5f51396..eae791b1feb6c6b02fa1dd021a08d9aab73bf82e

Much appreciated!

MDrakos
MDrakos previously approved these changes Nov 1, 2023
@mitchell-as mitchell-as closed this Nov 2, 2023
@mitchell-as mitchell-as reopened this Nov 2, 2023
@mitchell-as
Copy link
Contributor Author

@MDrakos Please re-approve. I had to perform a merge that conflicted on imports that was fixed here: 9403510#diff-c901c5791c0890edb63dc96809aa643617ebb5cd307e8877bf8c791ccf88147cR17.

@mitchell-as mitchell-as requested a review from MDrakos November 2, 2023 14:52
@mitchell-as mitchell-as merged commit da528fd into version/0-42-0-RC1 Nov 2, 2023
7 checks passed
@mitchell-as mitchell-as deleted the mitchell/dx-2303 branch November 2, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants