Support old project format with commitID in activestate.yaml. #2829

mitchell-as · 2023-10-18T18:16:24Z

DX-2263 As a State Tool user, I should be able to use my project with the old commit ID format.

Instead of automatically migrating projects, prompt the user. Projects in the old format are read-only.

mitchell-as · 2023-10-18T19:58:46Z

Test failures are not due to this PR, but are known, existing issues and the usual timeouts.

cmd/state/main.go

Naatan · 2023-10-18T20:28:40Z

internal/locale/locales/en-us.yaml

+    Your project is currently read-only until you migrate to the new project format.
+    After you migrate, older versions of the State Tool will not be able to use your project until they update.
+    Would you like to perform the migration now?


@rawktron please review

What's the full message? We have to give them some context for this. Is there another part of this message?

@rawktron That is the full message. Whenever they run a state command that needs to read a commitId from activestate.yaml, this message is presented to them. The ticket did not specify what the prompt should look like, so I came up with this for the time being. Please advise what message you'd like to give.

It should be something like "State Tool has introduced a new project format to provide enhanced functionality. Your project is currently using the old format and will remain read-only until you migrate to the new project format.

WARNING: After you migrate, older versions of the State Tool will not be able to use your project.

Would you like to perform the migration now?"

internal/runbits/projectmigration/projectmigration.go

pkg/localcommit/localcommit.go

test/integration/project_migration_int_test.go

Naatan · 2023-10-19T17:24:35Z

internal/runbits/projectmigration/projectmigration.go

+func Register(prompt_ promptable, out_ output.Outputer) {
+	prompt = prompt_
+	out = out_
+}


This is an antipattern for how we do this type of thing in our codebase. Either we should pass around a pointed struct that contains this context, or we always require these variables to be passed when invoking PromptAndMigrate.

That said I can see why you went this route as it feels really wasteful to add all this boilerplate for effectively a legacy use-case.

I feel like we need a pattern for handling legacy transitions like this, but that's probably too much to handle within the scope of this story.

Perhaps for this story we should clearly call out its legacy nature. I'm thinking add comments here clearly explaining that this is an antipattern and it's only ok because this is for a legacy use-case. I'm also thinking we should clearly mark packages meant for legacy use-cases as such, maybe by putting them a dir deeper under a parent dir called "legacy"? So you'd have runbits/legacy/projectmigration.

I'm not a big fan of any of these solutions, but I don't see a solution that doesn't have downsides. Main things I want to achieve here is maintainable code that doesn't start infecting other code, which I think is also where you were coming from. Happy to iterate with other ideas.

The need for this arose because we were auto-migrating the project as soon as it was read, which did not allow for any prompt/output parameter passing (i.e. projectfile.Parse(configFilePath)), especially since analytics was not available yet (prompt needs this, and analytics needs to know the project; circular dependency).

However, after a couple of iterations, we're making prompting a lazy action, so we can pass around prompter and output interfaces as normal. It's quite verbose, but I've made the change.

From what I understand, there's no need for a legacy/ parent package.

Naatan · 2023-10-19T17:29:11Z

pkg/project/project.go

 func (p *Project) Namespace() *Namespaced {
-	commitID, err := localcommit.GetCompatible(p)
+	commitID, err := localcommit.Get(p.Dir())


It just occurred to me we should not be exposing the commit ID through here at all. Could you see if it's easy to drop this from here or if not file a followup story? Effectively the project package should have zero awareness of localcommit, only of legacy commit.

Sure. After looking through the codebase, I cannot find a legitimate use for it anymore.

Naatan · 2023-10-19T18:00:02Z

internal/runbits/commitid/commitid.go

@@ -0,0 +1,33 @@
+package commitid


Since this invoked the prompter I think it makes sense you put it in as a runbit, though the naming of the package and method is triggering my OCD a little.

Open to other naming, but how about commitmediator.Get()?

I had no clue what name to use, so I'm glad for your suggestion.

Also renamed the package from the previous commit.

mitchell-as · 2023-10-19T20:43:14Z

pkg/projectfile/projectfile_test.go

-func TestMigrateCommitFromASY(t *testing.T) {
-	tempDir := fileutils.TempDirUnsafe()
-	defer os.RemoveAll(tempDir)
-
-	commitID := "7BA74758-8665-4D3F-921C-757CD271A0C1"
-	asy := filepath.Join(tempDir, constants.ConfigFileName)
-	err := fileutils.WriteFile(asy, []byte("project: https://platform.activestate.com/Owner/Name?branch=main&commitID="+commitID))
-	require.NoError(t, err)
-
-	proj, err := Parse(asy)
-	require.NoError(t, err)
-	assert.Equal(t, "Owner", proj.Owner())
-	assert.Equal(t, "Name", proj.Name())
-	assert.Equal(t, "main", proj.BranchName())
-
-	commitIdFile := filepath.Join(tempDir, constants.ProjectConfigDirName, constants.CommitIdFileName)
-	require.FileExists(t, commitIdFile)
-	assert.Equal(t, commitID, string(fileutils.ReadFileUnsafe(commitIdFile)))
-
-	assert.NotContains(t, string(fileutils.ReadFileUnsafe(asy)), commitID)
-}


Migration is no longer automatic. The new integration test covers this.

Naatan

I'm sorry, but as I expressed during stand I wanted to avoid adding this much boilerplate for the purpose of a legacy transition mechanic. This is adding a ton of complexity and code that will have to be maintained and any new code will also have to satisfy this pattern, all to facilitate migration logic for when grabbing a commit ID.

I'm open to other solutions aside from the one I suggested earlier, but the design goal here is not to have the legacy handling code introduce tons of boilerplate or add complexity whenever we need to access a commit.

This feels like the type of problem that we can't have a non-compromising solution for, at least now without heavy investment, which we want to avoid for this story.

Reverts 312e01a

…ern.

mitchell-as · 2023-10-20T14:56:29Z

Based on your feedback, you left the door open to interpretation. Either avoid the anti-pattern and pass things around (which is what we should be doing), or call it out and make legacy packages, etc. You acknowledged that there is no solution that does not have downsides. Since your first bit of feedback was "this is an anti-pattern, we should be passing things around", I figured that was your preference. If I had known you didn't want parameter passing, I wouldn't have done it.

Naatan · 2023-10-20T16:37:16Z

internal/runbits/legacy/projectmigration/projectmigration.go

+		return false, errs.Wrap(err, "Could not create local commit file")
+	}
+
+	if fileutils.DirExists(filepath.Join(proj.Dir(), ".git")) {


Realize this probably wasn't introduced here but we should go up the directory tree to check for this also, since people may not checkout their activestate.yaml inside the same dir as their git repo (eg. TheHomeRepot).

You can use:

cli/internal/fileutils/fileutils.go

Line 422 in 35d1e55

func FindFileInPath(dir, filename string) (string, error) {

Sadly, that's files only, not directories. Still, I'll do something to walk up the tree.

Naatan · 2023-10-20T16:39:54Z

internal/runbits/legacy/projectmigration/projectmigration.go

+	if fileutils.DirExists(filepath.Join(proj.Dir(), ".git")) {
+		err := localcommit.AddToGitIgnore(proj.Dir())
+		if err != nil {
+			multilog.Error("Unable to add local commit file to .gitignore: %v", err)


This would almost certainly be due to something client-side like permission errors I think? We should probably not log this to rollbar. Or maybe add a check to only log if it's not a permission or IO error.

I wasn't able to find a generic ErrIO variable, but I did find ErrPermission, so we'll go with that.

Naatan · 2023-10-20T16:42:22Z

pkg/project/project.go

+	commitId := strfmt.UUID(p.projectfile.LegacyCommitID())
+	return &Namespaced{p.projectfile.Owner(), p.projectfile.Name(), &commitId, false}


Is the legacy commit ID still required as part of the namespace? Suggest dropping it altogether from the namespace.

Having an eye towards https://activestatef.atlassian.net/jira/software/c/projects/DX/issues/DX-2289, I'd say we probably shouldn't drop it yet.

Never mind. We abandoned during stand, so I removed it. Let's see what breaks.

Naatan

Looks good! Thanks for sticking with it.

mitchell-as added the Test: critical label Oct 18, 2023

mitchell-as force-pushed the mitchell/dx-2263 branch from 54ef0b1 to 59be65d Compare October 18, 2023 18:50

Support old project format with commitID in activestate.yaml.

95b3728

Instead of automatically migrating projects, prompt the user. Projects in the old format are read-only.

mitchell-as force-pushed the mitchell/dx-2263 branch from 59be65d to 95b3728 Compare October 18, 2023 19:14

Updated non-migrated-project integration tests.

6fe2a50

mitchell-as requested a review from Naatan October 18, 2023 19:58

mitchell-as marked this pull request as ready for review October 18, 2023 19:58

Naatan requested changes Oct 18, 2023

View reviewed changes

Moved legacy commit id fetching into its own package.

94c1b58

mitchell-as requested a review from Naatan October 18, 2023 22:14

Naatan requested changes Oct 19, 2023

View reviewed changes

Pass around prime prompt and output instead of using globals.

312e01a

Also renamed the package from the previous commit.

mitchell-as force-pushed the mitchell/dx-2263 branch from c05892c to 312e01a Compare October 19, 2023 19:44

Project package should not reference the localcommit package.

b700527

mitchell-as force-pushed the mitchell/dx-2263 branch from 15ad7f5 to b700527 Compare October 19, 2023 20:11

mitchell-as requested a review from Naatan October 19, 2023 20:12

Fixed unit test failures.

82a914d

mitchell-as commented Oct 19, 2023

View reviewed changes

Naatan requested changes Oct 19, 2023

View reviewed changes

mitchell-as added 2 commits October 20, 2023 10:13

Revert: Pass around prime prompt and output instead of using globals.

bbda2a4

Reverts 312e01a

Rename packages and added comments explaining the use of an anti-patt…

869caa9

…ern.

mitchell-as requested a review from Naatan October 20, 2023 15:02

Update project migration prompt.

aa62fe5

Naatan requested changes Oct 20, 2023

View reviewed changes

mitchell-as added 2 commits October 20, 2023 13:11

Project migration should walk up directories when looking for .git/.

394a7f9

Merge branch 'version/0-42-0-RC1' into mitchell/dx-2263

df44957

mitchell-as requested a review from Naatan October 20, 2023 17:29

Do not include legacy commitId when returning project namespace.

19f9f31

Naatan approved these changes Oct 20, 2023

View reviewed changes

mitchell-as removed the Test: critical label Oct 20, 2023

mitchell-as closed this Oct 20, 2023

mitchell-as reopened this Oct 20, 2023

mitchell-as merged commit 17ec0f9 into version/0-42-0-RC1 Oct 20, 2023
9 of 12 checks passed

mitchell-as deleted the mitchell/dx-2263 branch October 20, 2023 18:50

as-builds added a commit that referenced this pull request Oct 20, 2023

Merge branch version/0-42-0-RC1 to adopt changes from PR #2829

b54d960

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support old project format with commitID in activestate.yaml. #2829

Support old project format with commitID in activestate.yaml. #2829

mitchell-as commented Oct 18, 2023 •

edited by github-actions bot

Loading

mitchell-as commented Oct 18, 2023

Naatan Oct 18, 2023

rawktron Oct 20, 2023

mitchell-as Oct 20, 2023

rawktron Oct 20, 2023

Naatan Oct 19, 2023

mitchell-as Oct 19, 2023 •

edited

Loading

Naatan Oct 19, 2023

mitchell-as Oct 19, 2023

Naatan Oct 19, 2023

mitchell-as Oct 19, 2023

mitchell-as Oct 19, 2023

Naatan left a comment

mitchell-as commented Oct 20, 2023

Naatan Oct 20, 2023

mitchell-as Oct 20, 2023

Naatan Oct 20, 2023

mitchell-as Oct 20, 2023

Naatan Oct 20, 2023

mitchell-as Oct 20, 2023

mitchell-as Oct 20, 2023

Naatan left a comment

		commitId := strfmt.UUID(p.projectfile.LegacyCommitID())
		return &Namespaced{p.projectfile.Owner(), p.projectfile.Name(), &commitId, false}

Support old project format with commitID in activestate.yaml. #2829

Support old project format with commitID in activestate.yaml. #2829

Conversation

mitchell-as commented Oct 18, 2023 • edited by github-actions bot Loading

mitchell-as commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naatan left a comment

Choose a reason for hiding this comment

mitchell-as commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naatan left a comment

Choose a reason for hiding this comment

mitchell-as commented Oct 18, 2023 •

edited by github-actions bot

Loading

mitchell-as Oct 19, 2023 •

edited

Loading