Transitioning to buildscript is handled explicitly #3199

Naatan · 2024-03-26T21:22:43Z

DX-2619 Transitioning to buildscripts / build expressions is handled explicitly

This is addressing several things:

Buildscripts are created explicitly; either because the project config needs migrating, or by running state reset.
Project config now has a migration mechanic
Runtime will error out if buildscripts are opted in but no buildscripts exist; you'll get an error informing you to run state reset LOCAL
Runtime runbits were refactored to ensure this new buildscript error is properly propagated in all scenarios

# Conflicts: # internal/locale/locales/en-us.yaml # internal/runbits/refresh.go # internal/runbits/runtime/runtime.go # internal/runners/deploy/deploy.go # internal/runners/exec/exec.go # internal/runners/prepare/prepare.go # internal/runners/reset/reset.go # internal/scriptrun/scriptrun.go # pkg/platform/runtime/runtime.go

mitchell-as · 2024-03-29T15:45:08Z

If I opt into buildscripts and then attempt to checkout a project, it will immediately fail:

% state checkout ActiveState-CLI/Merge#447b8363-024c-4143-bf4e-c96989314fdf
Checking out project: ActiveState-CLI/Merge

█ Installing Runtime

 x Your project is missing its buildscript file, please run 'state reset LOCAL' to recreate it.

If I prepend ACTIVESTATE_CLI_DISABLE_RUNTIME=true to the checkout, it will checkout, but then attempting to do something with the runtime (e.g. state install requests) will fail with an unhelpful error message:

% state install requests
█ Installing Package

Operating on project ActiveState-CLI/Merge, located at /Users/me/tmp/Merge.

 • Searching for requests in the ActiveState Catalog ✔ Found
 • Creating commit ✔ Done
 • Resolving Dependencies x Failed
█ Something Went Wrong

 x Could not initialize runtime.

Apparently I need to run state reset LOCAL first. This is very awkward.

MDrakos

This is a large PR so I tried to provide some higher level feedback. Overall the migration implementation looks good to me and makes sense.

MDrakos · 2024-04-01T17:03:13Z

internal/runbits/requirements/requirements.go

-	if err != nil {
-		return errs.Wrap(err, "Could not solve runtime")
-	}
+	if strings.ToLower(os.Getenv(constants.DisableRuntime)) != "true" {


nit: It's unfortunate that we have to have this here. Currently, outside of tests, this environment variable is only checked in the runtime package.

Yeah agreed, but I don't see a way to address this without significant refactoring, which is what we're already planning on, just not for this story.

That's fair, I didn't want to tag this as anything other than a nit because I know we have the refactor coming.

MDrakos · 2024-04-01T17:10:51Z

internal/runbits/runtime/rationalize.go

 	// If updating failed due to unidentified errors, and the user is not authenticated, add a tip suggesting that they authenticate as
 	// this may be a private project.
 	// Note since we cannot assert the actual error type we do not wrap this as user-facing, as we do not know what we're
 	// dealing with so the localized underlying errors are more appropriate.
-	case isUpdateErr && !auth.Authenticated():
+	case !auth.Authenticated(): // MUST BE LAST


Because this is not asserting an error, it's decorating an unknown error. And all the other assertions are for asserting an error.

I've updated the code with a comment and some protection against accidental refactoring.

Note the original implementation was bugged:

https://github.com/ActiveState/cli/pull/3199/files/3c9e66d2268323d48391a6b19658bd20b4a7e327#diff-ad4a4ce35107eceb3ee56dfede324f0f7eb85af84005dbd6dc158f5681e58c92L28

It was ignoring errors if those non-error assertions were evaluated. We really should void using rationalizers to do anything that's not error assertion related.

MDrakos · 2024-04-01T17:16:57Z

internal/runbits/runtime/refresh.go

+
+const (
+	OptNone Opts = 1 << iota
+	OptMinimalUI


Shouldn't this be something that is set in the outputter or in some sort of output layer? From reading just this file I'm not sure what this means and why it's coupled with the order changing, at least in terms of options.

I'll add comments. I have no intention of using this outside of this package, or even beyond the runtime refactor. This is effectively a temporary workaround to get around the increasingly lacking architecture.

MDrakos · 2024-04-01T17:28:31Z

internal/runners/reset/reset.go

-	err = runtime.RefreshRuntime(r.auth, r.out, r.analytics, r.project, commitID, true, target.TriggerReset, r.svcModel, r.cfg)
+	// Ensure the buildscript exists. Normally we should never do this, but reset is used for resetting from a corrupted
+	// state, so it is appropriate.
+	if r.cfg.GetBool(constants.OptinBuildscriptsConfig) {


Have we considered centralizing the check of this config option to the buildscript package? Similar to how the DisableRuntime environment variable has worked before. It looks like every use, at least in this PR is followed by a call out to that package.

It's not really the same thing, the runtime instance is still getting called regardless of the env var, it just behaves differently. In this case we're not doing anything with the buildscript outside of this one call. Add to that the fact that we have some commands like commit and eval that do not care about this config, and it just makes more sense to have it in the runner.

MDrakos · 2024-04-01T18:40:18Z

pkg/platform/api/buildplanner/model/buildplan.go

@@ -266,39 +282,51 @@ func (b *BuildPlanByProject) CommitID() (strfmt.UUID, error) {
 }

 type BuildPlanByCommit struct {
-	Commit *Commit `json:"commit"`
+	commit *Commit `json:"commit"`


I believe that by making this field private the encoding/json package will not be able to access it so marhsalling/unmarshalling may fail. Maybe that's changed but I remember getting caught by this before.

Urgh you're right, thanks for catching!

MDrakos · 2024-04-01T18:46:00Z

pkg/projectfile/projectfile.go

+		if project.ConfigVersion > ConfigVersion {
+			return nil, locale.NewInputError("err_projectfile_version_too_high")
+		}
+		updatedConfigVersion, errMigrate := migrator(project, ConfigVersion)


We will lose this error if anything on line 483 fails. We may want to log it in that case if this could be valuable.

Good catch! Perfect job for errs.Pack().

MDrakos · 2024-04-01T18:46:52Z

pkg/projectfile/yamlfield.go

+	return &yamlField{field: field, value: value}
+}
+
+func (y *yamlField) Save(path string) error {


nit: this feels low level enough to have a unit test.

# Conflicts: # internal/locale/locales/en-us.yaml # internal/runners/packages/list.go # internal/runners/swtch/switch.go

MDrakos

Looks good to me! Just left a couple of minor comments, not meant to hold up the PR.

internal/runbits/runtime/rationalize.go

MDrakos · 2024-04-02T22:20:55Z

pkg/platform/runtime/setup/buildlog/buildlog.go

+				if _, ok := artifactsDone[m.ArtifactID]; ok {
+					multilog.Critical("buildlogstreamer sent duplicate ArtifactSucceededMessage for artifact %s", m.ArtifactID.String())
+					break
+				}


Is this meant to be a part of another story?

Huh, yeah.. I had run git reset, not sure why this is still in :\

Co-authored-by: Mike Drakos <miked@activestate.com>

This reverts commit 7616dea.

This reverts commit 1c980ad.

Naatan added 11 commits March 21, 2024 13:15

Take out implicit buildscript creation

cfa341a

Added migration mechanic for adding buildscript

7da903e

Reset creates buildscript if it does not exist

8e5318d

Move runbit logic to appropriate subpkg

e96c771

Fix merge issues

23f4798

Centralize runbit logic and handle errors

5dec436

Store buildscript during runtime sourcing

6cd23fe

Cleanup

08ac4fc

Fix runtime refreshing always causing an error when not authenticated

d96e7b8

Fix runtime errors marked as input error without proper cause

14aeb9e

Naatan added Test: activate Test: critical Test: shell Test: refresh Test: install Test: reset labels Mar 26, 2024

Naatan changed the base branch from master to version/0-44-0-RC1 March 26, 2024 21:24

Naatan added 12 commits March 26, 2024 14:25

Fix buildscript reset error not fired correctly

e49c759

Merge branch 'version/0-44-0-RC1' into DX-2619

4decb79

Fix activate using wrong project

5a048bc

Fix projectfile not usable due to migrator not being set in unit tests

ba5e79e

Fix reset error not surfaced if no cached buildscript file exists

2ad7d24

Drop unused param

138aa48

Add buildscript test to cover reset scenario

db07765

Update test assertion to match updated implementation

9af6b88

Handle artifact build errors with more detail

4aa5a88

Only create buildscript if opted in

d332b3d

Include log URI in source error

a1620d2

Ensure package operations support disabled runtimes

3e4d250

Debug artifact status

3c9e66d

Naatan added Test: deploy Test: init Test: checkout labels Mar 28, 2024

Naatan requested a review from MDrakos March 28, 2024 16:43

MDrakos requested changes Apr 1, 2024

View reviewed changes

Naatan added 8 commits April 2, 2024 10:09

Checkout should create buildscript

fdf3ad0

Create buildscript on init

a7ea6ee

Tag buildscript tests

e04dfc2

Ensure auth tip is a last resort, and add comment explaining why

ebe1bfd

Add comments

79eb4fb

Ensure commit field is marshalled

64302ce

Ensure both errors are surfaced

fd42834

Add test for yamlfield

fe3e708

Naatan added the Test: buildscripts label Apr 2, 2024

Naatan added 3 commits April 2, 2024 12:52

Merge branch 'version/0-44-0-RC1' into DX-2619

bdbc455

# Conflicts: # internal/locale/locales/en-us.yaml # internal/runners/packages/list.go # internal/runners/swtch/switch.go

Wrap errors so we can tell from the log which path was used

7616dea

Handle duplicate artifact success events at the source

1c980ad

Naatan requested a review from MDrakos April 2, 2024 21:51

MDrakos previously approved these changes Apr 2, 2024

View reviewed changes

Naatan dismissed MDrakos’s stale review via b9a6533 April 2, 2024 22:23

Naatan and others added 3 commits April 2, 2024 15:23

Update internal/runbits/runtime/rationalize.go

b9a6533

Co-authored-by: Mike Drakos <miked@activestate.com>

Revert "Wrap errors so we can tell from the log which path was used"

0218098

This reverts commit 7616dea.

Revert "Handle duplicate artifact success events at the source"

4a1a858

This reverts commit 1c980ad.

Naatan requested a review from MDrakos April 2, 2024 22:26

MDrakos approved these changes Apr 2, 2024

View reviewed changes

Naatan merged commit 7ba7ef6 into version/0-44-0-RC1 Apr 2, 2024
4 of 6 checks passed

Naatan deleted the DX-2619 branch April 2, 2024 22:33

as-builds added a commit that referenced this pull request Apr 2, 2024

Merge branch version/0-44-0-RC1 to adopt changes from PR #3199

5b266a0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transitioning to buildscript is handled explicitly #3199

Transitioning to buildscript is handled explicitly #3199

Naatan commented Mar 26, 2024 •

edited by github-actions bot

Loading

mitchell-as commented Mar 29, 2024 •

edited

Loading

MDrakos left a comment

MDrakos Apr 1, 2024

Naatan Apr 2, 2024

MDrakos Apr 2, 2024

MDrakos Apr 1, 2024

Naatan Apr 2, 2024

MDrakos Apr 1, 2024

Naatan Apr 2, 2024

MDrakos Apr 1, 2024

Naatan Apr 2, 2024

MDrakos Apr 1, 2024

Naatan Apr 2, 2024

MDrakos Apr 1, 2024

Naatan Apr 2, 2024

MDrakos Apr 1, 2024

MDrakos left a comment

MDrakos Apr 2, 2024

Naatan Apr 2, 2024

Transitioning to buildscript is handled explicitly #3199

Transitioning to buildscript is handled explicitly #3199

Conversation

Naatan commented Mar 26, 2024 • edited by github-actions bot Loading

mitchell-as commented Mar 29, 2024 • edited Loading

MDrakos left a 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

Choose a reason for hiding this comment

MDrakos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naatan commented Mar 26, 2024 •

edited by github-actions bot

Loading

mitchell-as commented Mar 29, 2024 •

edited

Loading