Initial implementation of `state checkout --from-archive`. #3444

mitchell-as · 2024-08-12T21:54:20Z

DX-2981 `state checkout` can install from an offline source

We needed a project.json file, similar to offline installers, so here's a sample of the one I'm using in integration tests:

{
	"org_name": "ActiveState-CLI",
	"project_name": "AlmostEmpty",
	"commit_id": "6cd1cc1f-3886-439b-8373-c24ca06ab150",
	"branch": "main",
	"platform_id": "78977bc8-0f32-519d-80f3-9043f059398c"
}

… need it.

mitchell-as · 2024-08-13T16:27:45Z

The lone test failure is a timeout unrelated to this PR.

…network to complete.

Naatan · 2024-08-14T17:08:13Z

cmd/state/internal/cmdtree/checkout.go

+			{
+				Name:        "from-archive",
+				Description: locale.Tl("flag_state_checkout_from_archive", "Checkout from the given .tar.gz archive"),
+				Value:       &params.FromArchive,
+			},


I noticed the manual verification you're doing where essentially either namespace is required OR this flag is. This makes me think maybe we should treat the namespace arg more intelligently. eg. if you specify a filepath for the namespace then we use that. What do you think?

Since we use a special type for this arg we could introduce a new type that multiplexes to either namespace or filepath.

That sounds reasonable, as long as the archive path always has an extension. Otherwise the argument is ambiguous, as it can be either org/project, project, or path/to/tar.gz.

Sounds good, I don't see any reason why it wouldn't have an extension.

Naatan · 2024-08-14T17:13:25Z

internal/runbits/checkout/archive.go

+// readProject reads and returns a project namespace (with commitID) and branch from
+// "project.json", as well as a platformID.
+func readProject(dir string) (*project.Namespaced, string, strfmt.UUID, error) {
+	projectBytes, err := fileutils.ReadFile(filepath.Join(dir, "project.json"))


Please make project.json a constant, and rename it to installer_config.json.

Naatan · 2024-08-14T17:14:16Z

internal/runbits/checkout/archive.go

+		return nil, errs.Wrap(err, "Invalid archive: buildplan.json not found")
+	}
+
+	return buildplan.Unmarshal(buildplanBytes)


Still need to wrap this error.

Naatan · 2024-08-14T17:17:56Z

internal/runbits/checkout/checkout.go

 	commitID := ns.CommitID
+	var language string
+	if !fromArchive {


Lets extract the logic under this to its own function, as the bulk of the current function is inside this logic statement.

Naatan · 2024-08-14T17:23:34Z

internal/runbits/runtime/runtime.go

+	var buildPlan *buildplan.BuildPlan
+	if opts.Archive != nil {
+		buildPlan = opts.Archive.BuildPlan
+	} else if opts.Commit != nil {
+		buildPlan = opts.Commit.BuildPlan()
+	}


Suggest making this a switch case, and making the default behaviour the logic from line 187. Just so the relations are clearly communicated.

Naatan · 2024-08-14T17:24:18Z

internal/runbits/runtime/runtime.go

-	); err != nil {
+	}
+	if opts.Archive != nil {
+		rtOpts = append(rtOpts, runtime.WithFromArchive(opts.Archive.Dir, opts.Archive.PlatformID, checkout.ArtifactExt))


Can rename this one to WithArchive imo. The "From" isn't doing anything but trigger my OCD :p

Naatan · 2024-08-14T17:28:03Z

internal/runbits/checkout/checkout.go

 func New(repo git.Repository, prime primeable) *Checkout {
 	return &Checkout{repo, prime.Output(), prime.Config(), prime.Analytics(), "", prime.Auth()}
 }

-func (r *Checkout) Run(ns *project.Namespaced, branchName, cachePath, targetPath string, noClone bool) (_ string, rerr error) {
+func (r *Checkout) Run(ns *project.Namespaced, branchName, cachePath, targetPath string, noClone, fromArchive bool) (_ string, rerr error) {


The checkout runbit doesn't seem to actually care about archives. I think we should name this according to the behaviour of the package, not according to the use-case we have external from this package. So perhaps we can name this validate, as that's what the logic seems to do (with a little bit of cleanup sprinkled in there, so maybe validateAndCleanup?).

I've gone with onlyValidate. With all of the requested changes, this makes sense to me, but let me know if you disagree and we can reconsider your suggestions.

Naatan · 2024-08-14T20:04:21Z

pkg/runtime/setup.go

+	// Options for setting up a runtime from an archive.
+	FromArchiveDir string
+	PlatformID     *strfmt.UUID
+	ArtifactExt    string


Please make a struct to put these under. As they are all directly related and should never be used independently from one another.

Naatan · 2024-08-14T20:06:17Z

pkg/runtime/setup.go

+		var err error
+		name := artifact.ArtifactID.String() + s.opts.ArtifactExt
+		artifactFile := filepath.Join(s.opts.FromArchiveDir, name)
+		logging.Debug("Reading file '%s' for '%s'", artifactFile, artifact.DisplayName)
+		b, err = fileutils.ReadFile(artifactFile)
+		if err != nil {
+			return errs.Wrap(err, "read from archive failed")
+		}


I think since this is bypassing the event reporting of downloads we will have a progress indicator for downloads that never moves.

We'll want to ensure that those events are still send, but we probably also want to update our runbit logic so that instead of "Downloading" we say "Unpacking" when using the archive approach.

Okay, I'll add the download events. In testing, this step went by so fast I didn't even see a download bar. I only saw a brief "Unpacking" bar since setting up from an archive does go through this step.

Naatan · 2024-08-14T20:09:59Z

test/integration/checkout_int_test.go

+	cp.Expect("Installing")
+	cp.Expect("All dependencies have been installed and verified")
+	cp.Expect("Checked out project ActiveState-CLI/AlmostEmpty")
+	cp.ExpectExitCode(0)


Let's also assert that the resulting runtime has some of the expected files. This test currently is just testing the UI/UX of the command itself, we should also verify that the end result is what we expect it to be.

Now the namespace argument can be either - org/project - project (org is inferred) - archive.tar.gz

Naatan · 2024-08-15T16:39:49Z

internal/runbits/checkout/checkout.go

+// checkout performs the checkout and returns the checkout's owner, project, commitID, branchName,
+// and language.
+func (r *Checkout) checkout(


It doesn't feel right to call this checkout. That's what the main Run function does (ie. creating the project file is effectively the checkout).

Suggest turning this into a function called sanitize() that does all the cleaning up of parameters, and then separately call the clone behaviour from Run().

I'm not a big fan of the number of parameters this returns, but I also don't see an obvious solution that doesn't add boilerplate that I'm even less of a fan of. Just flagging it in case you have ideas.

I've gone with fetchProject since I feel like that is the goal of this function. Fetch the project from the platform and return its complete set of parameters. If you don't like it, we'll go with sanitize.

Naatan · 2024-08-15T16:41:38Z

internal/runbits/checkout/checkout.go

 }

-func (r *Checkout) Run(ns *project.Namespaced, branchName, cachePath, targetPath string, noClone, fromArchive bool) (_ string, rerr error) {
+func (r *Checkout) Run(ns *project.Namespaced, branchName, cachePath, targetPath string, noClone, onlyValidate bool) (_ string, rerr error) {


onlyValidate doesn't seem an appropriate name to me. If supplied it in fact does no validations, and it only does the checkout (ie. create the project files).

Perhaps bareCheckout would be more appropriate?

Okay. I don't want to spend too much time on a name.

Naatan · 2024-08-15T16:43:13Z

internal/runners/checkout/checkout.go

+		err := ns.Set(params.Namespace)
+		if err != nil {


Suggested change

err := ns.Set(params.Namespace)

if err != nil {

if err := ns.Set(params.Namespace); err != nil {

Naatan · 2024-08-15T16:46:33Z

internal/runners/checkout/checkout.go

 		if err != nil {
 			return errs.Wrap(err, "Unable to read archive")
 		}
 		defer archive.Cleanup()
-		params.Namespace = archive.Namespace
+		ns = *archive.Namespace


Suggested change

ns = *archive.Namespace

ns = archive.Namespace

Doesn't seem work dereferencing when all your use of it doesn't seem to care, and in fact you create a reference again when passing it to checkout.Run(). May as well avoid the potential nil pointer.

ns cannot be a pointer because of the ns.Set() function used earlier. The dereference here is needed, as is the passing of &ns to checkout.Run(). It's a product of our clever use the first argument being either a namespace or archive file.

Naatan · 2024-08-15T16:47:14Z

pkg/runtime/options.go

-		opts.ArtifactExt = ext
-	}
+func WithArchive(dir string, platformID strfmt.UUID, ext string) SetOpt {
+	return func(opts *Opts) { opts.FromArchive = &fromArchive{dir, platformID, ext} }


Suggested change

return func(opts *Opts) { opts.FromArchive = &fromArchive{dir, platformID, ext} }

return func(opts *Opts) {

opts.FromArchive = &fromArchive{dir, platformID, ext}

}

Naatan · 2024-08-15T16:49:43Z

pkg/runtime/setup.go

+
+		if err := s.fireEvent(events.ArtifactDownloadSuccess{artifact.ArtifactID}); err != nil {
+			return errs.Wrap(errs.Pack(err, err), "Could not handle ArtifactDownloadSuccess event")
+		}


I just realized we are in fact handling unpack events separately. So I think it makes more sense that we don't send the download events and instead set the toDownload counter to 0 if we are installing from an archive, ie.

cli/pkg/runtime/setup.go

Line 114 in 6ff1b5a

artifactsToDownload := artifactsToInstall.Filter(func(a *buildplan.Artifact) bool {

Apologies, I didn't have this in mind with my initial review.

Okay, well that complicated things. Let me know if my solution is sub-optimal.

Naatan · 2024-08-15T16:53:46Z

test/integration/checkout_int_test.go

+	proj, err := project.FromPath(filepath.Join(ts.Dirs.Work, "AlmostEmpty"))
+	suite.Require().NoError(err)
+	cachePath := filepath.Join(ts.Dirs.Cache, runtime_helpers.DirNameFromProjectDir(proj.Dir()))
+	zlibH := filepath.Join(cachePath, "usr", "include", "zlib.h")


I'm pretty sure this will be different on mac/windows.

Actually, it can be whatever I say it is since I produced the test archives :) But seriously, I untarred the ingredients and looked for a common one that had the same path and used that. The tests on all three platforms are passing, so it's all good.

Oh right, I forgot about that detail :p Ok we'll worry about this once we can test with proper payloads.

mitchell-as · 2024-08-16T17:33:38Z

Closing in favor of #3451

Review iteration DX-2981

Naatan · 2024-08-16T18:07:29Z

@mitchell-as reopened as that other ticket was targeting this one so the review process was still workable. Though given I'm about to approve I guess this is just busywork 😅 Anyway now you have a signal to move the story along.

mitchell-as added the Test: checkout label Aug 12, 2024

mitchell-as force-pushed the mitchell/dx-2981 branch 2 times, most recently from c2d9352 to c896512 Compare August 13, 2024 15:09

Initial implementation of state checkout --from-archive.

5b48da3

mitchell-as force-pushed the mitchell/dx-2981 branch from c896512 to 5b48da3 Compare August 13, 2024 15:53

Make org/namespace an optional argument since --from-archive does not…

89dd3ec

… need it.

mitchell-as force-pushed the mitchell/dx-2981 branch from ff925c3 to 29e2bbb Compare August 13, 2024 17:15

state checkout --from-archive integration test should not need the …

78c04c7

…network to complete.

mitchell-as force-pushed the mitchell/dx-2981 branch from 29e2bbb to 78c04c7 Compare August 13, 2024 18:11

mitchell-as requested a review from Naatan August 13, 2024 21:06

Naatan requested changes Aug 14, 2024

View reviewed changes

mitchell-as added 3 commits August 14, 2024 17:07

Allow passing checkout archive as namespace argument.

59bf4a7

Now the namespace argument can be either - org/project - project (org is inferred) - archive.tar.gz

Use constants for checkout archive files.

1a878e6

Wrap error.

08b1feb

mitchell-as force-pushed the mitchell/dx-2981 branch 2 times, most recently from dfbedd0 to cbcfa75 Compare August 14, 2024 22:34

mitchell-as marked this pull request as ready for review August 14, 2024 22:47

mitchell-as added 7 commits August 15, 2024 10:37

Extract method.

7cf19c8

Use switch to clarify relations.

dfdf8ac

Rename option for readability.

6b540bd

Group runtime setup from archive options into a single struct.

b889013

Still fire download events when setting up from an archive.

ba718c4

Assert checkout from archive installs runtime files.

78ffd03

Remove unused field.

9ad4079

mitchell-as force-pushed the mitchell/dx-2981 branch from cbcfa75 to 9ad4079 Compare August 15, 2024 14:38

mitchell-as added 2 commits August 15, 2024 10:54

Rename argument.

a03e3de

Remove debug logging.

f251055

mitchell-as requested a review from Naatan August 15, 2024 15:13

Naatan requested changes Aug 15, 2024

View reviewed changes

Applied PR feedback.

836f8a8

mitchell-as force-pushed the mitchell/dx-2981 branch 2 times, most recently from 9ea49f1 to 08ebfdf Compare August 15, 2024 19:36

Separate artifacts to download from artifacts to unpack from archive.

c193c9f

mitchell-as force-pushed the mitchell/dx-2981 branch from 08ebfdf to c193c9f Compare August 15, 2024 19:37

mitchell-as requested a review from Naatan August 15, 2024 19:51

Naatan added 2 commits August 16, 2024 10:09

Cleanup namespace detection

72d1f0b

Remove unnecessary logic

d694c07

mitchell-as closed this Aug 16, 2024

Merge pull request #3451 from ActiveState/nathan/DX-2981

e0de8ac

Review iteration DX-2981

Naatan reopened this Aug 16, 2024

Naatan approved these changes Aug 16, 2024

View reviewed changes

mitchell-as removed the Test: checkout label Aug 16, 2024

mitchell-as closed this Aug 16, 2024

mitchell-as reopened this Aug 16, 2024

mitchell-as merged commit 34cf30c into version/0-46-0-RC1 Aug 16, 2024
13 of 18 checks passed

mitchell-as deleted the mitchell/dx-2981 branch August 16, 2024 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of `state checkout --from-archive`. #3444

Initial implementation of `state checkout --from-archive`. #3444

mitchell-as commented Aug 12, 2024 •

edited by github-actions bot

Loading

mitchell-as commented Aug 13, 2024

Naatan Aug 14, 2024

mitchell-as Aug 14, 2024

Naatan Aug 15, 2024

Naatan Aug 14, 2024

Naatan Aug 14, 2024

Naatan Aug 14, 2024

Naatan Aug 14, 2024

Naatan Aug 14, 2024

Naatan Aug 14, 2024

mitchell-as Aug 15, 2024

Naatan Aug 14, 2024

Naatan Aug 14, 2024

mitchell-as Aug 14, 2024

Naatan Aug 14, 2024

Naatan Aug 15, 2024

mitchell-as Aug 15, 2024 •

edited

Loading

Naatan Aug 15, 2024

mitchell-as Aug 15, 2024 •

edited

Loading

Naatan Aug 15, 2024

Naatan Aug 15, 2024

mitchell-as Aug 15, 2024

Naatan Aug 15, 2024

Naatan Aug 15, 2024

mitchell-as Aug 15, 2024

Naatan Aug 15, 2024

mitchell-as Aug 15, 2024

Naatan Aug 15, 2024

mitchell-as commented Aug 16, 2024

Naatan commented Aug 16, 2024

	err := ns.Set(params.Namespace)
	if err != nil {
	if err := ns.Set(params.Namespace); err != nil {

Initial implementation of state checkout --from-archive. #3444

Initial implementation of state checkout --from-archive. #3444

Conversation

mitchell-as commented Aug 12, 2024 • edited by github-actions bot Loading

mitchell-as commented Aug 13, 2024

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

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

Choose a reason for hiding this comment

mitchell-as Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as Aug 15, 2024 • 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

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 commented Aug 16, 2024

Naatan commented Aug 16, 2024

Initial implementation of `state checkout --from-archive`. #3444

Initial implementation of `state checkout --from-archive`. #3444

mitchell-as commented Aug 12, 2024 •

edited by github-actions bot

Loading

mitchell-as Aug 15, 2024 •

edited

Loading

mitchell-as Aug 15, 2024 •

edited

Loading