Add separate checkout info block to buildscripts. #3514

mitchell-as · 2024-09-26T21:40:53Z

DX-3024 Non-versioned buildscript information is clearly denoted

Use triple-backticks to separate checkout info from buildscript. Using triple-quotes will require a new parser and/or lexer. Since we're experimenting, let's use what we have.
Buildscripts always have an AtTime (it's no longer optional). I believe we had it optional for compatiblity with build expressions that didn't have a separate commit time, but all build expressions now have an associated commit time from what I can tell.
Renamed localcommit package to checkoutinfo
- checkoutinfo is a primer property and all runners go through it to get commit IDs and set commit info like owner, name, and branch.
Changed buildplanner.GetBuildScript() to additionally accept an owner, project, branch, and commitID. It uses these fields to construct the "Project" field in the commit info section of buildscripts.
state reset LOCAL will always re-initialize a build script.
Internal change in the raw buildscript format to not store strings with quotes. (Unmarshaling strips quotes, and marshaling adds them back.)
Users are shown a warning if they attempt to use an outdate build script (state install, etc.).

Of the form: ``` Project: URL Time: time ```

…nfo. Centralize this so the buildscript and projectfile can remain in sync.

Naatan

Looking good! Nice work. Just need some slight iteration on smaller parts. The config change is probably the biggest, but ultimately should lead to an overall cleaner changeset (if not then I'm way off on that review comment; by all means call me on that).

Naatan · 2024-10-01T21:02:04Z

internal/runbits/buildplanner/buildplanner.go

+		pj, err := model.FetchProjectByName(namespace.Owner, namespace.Project, auth)
+		if err != nil {
+			return nil, locale.WrapExternalError(err, "err_fetch_project", "", namespace.String())
+		}
+
+		branch, err := model.DefaultBranchForProject(pj)
+		if err != nil {
+			return nil, errs.Wrap(err, "Could not grab branch for project")
+		}


We should provide this explicitly. Because we may not be on the default branch, and at this level we have on idea what the intend was. Consuming code won't know that this is setting up any branch related stuff.

Note we could just add an argument for the branch which can be nil, and then we default to this logic if it is nil. My concern is primarily about making the contract with the branch explicit with consuming code.

We can actually just pass "", and the build script checkout info will silently ignore it (like activestate.yaml with just a commitID and no branch). We cannot explicitly provide a branch argument because state export buildplan, state artifacts and state artifacts download only know about commit ID. We'd have to do branch lookup or ask the user to supply it on the command line. Either one is messy.

Let me know if you disagree.

Naatan · 2024-10-01T21:09:14Z

pkg/checkoutinfo/checkoutinfo.go

+		return nil
+	}
+
+	// Note: cannot use functions from buildscript_runbit due to import cycle.


Suggested change

// Note: cannot use functions from buildscript_runbit due to import cycle.

This is moot; we cannot (should not) use functions from runbits period.

Naatan · 2024-10-01T21:12:30Z

pkg/checkoutinfo/checkoutinfo.go

+	if !c.config.GetBool(constants.OptinBuildscriptsConfig) {
+		return nil
+	}


Please make this a boolean argument passed to new (optinBuidscripts bool), instead of evaluating it here and having a dependency on config.

Naatan · 2024-10-01T21:15:03Z

pkg/buildscript/buildscript.go

+	return b.raw.CheckoutInfo.Project
+}
+
+func (b *BuildScript) SetProject(url string) {
+	b.raw.CheckoutInfo.Project = url
+}
+
+func (b *BuildScript) AtTime() time.Time {
+	return b.raw.CheckoutInfo.AtTime
 }

 func (b *BuildScript) SetAtTime(t time.Time) {
-	b.raw.AtTime = &t
+	b.raw.CheckoutInfo.AtTime = t


I just realized; this probably shouldn't be on raw at all. raw covers the build expression portion, which this is not a part of.

That said I don't recall what entanglements exist. I'd suggest changing this to the buildscript (non-raw) package itself if it is trivial, otherwise we can get to it later.

Naatan · 2024-10-01T21:16:58Z

pkg/buildscript/merge.go

@@ -57,8 +57,8 @@ func (b *BuildScript) Merge(other *BuildScript, strategies *mono_models.MergeStr

 	// When merging build scripts we want to use the most recent timestamp
 	atTime := other.AtTime()
-	if atTime != nil && atTime.After(*b.AtTime()) {
-		b.SetAtTime(*atTime)
+	if atTime.After(b.AtTime()) {


I think we still need the nil check. Yes any new commits and realistically 99.9% of commits have the atTime var. But commits are final; we do not edit them. Meaning old commits without this are out there, and will forever be out there.

If this is a pain to support we could maybe look into erroring out, but in that scenario we need to outline to the user how they can get out of this broken state. I don't know what that would look like exactly, so if we want to remove support for older commits we'll need to at least file a story to address those older commits with an error.

Naatan · 2024-10-01T21:20:26Z

pkg/buildscript/unmarshal.go

+			return nil, locale.NewExternalError(
+				"err_buildscript_checkoutinfo",
+				"Could not parse checkout information in the buildscript. The parser produced the following error: {{.V0}}", err.Error())


Suggested change

return nil, locale.NewExternalError(

"err_buildscript_checkoutinfo",

"Could not parse checkout information in the buildscript. The parser produced the following error: {{.V0}}", err.Error())

return nil, locale.NewInputError(

"err_buildscript_checkoutinfo",

"Could not parse checkout information in the buildscript. The parser produced the following error: {{.V0}}", err.Error())

Also it seems like this error should be more helpful. This isn't just any parsing error; we know it relates specifically to the checkoutinfo section.

Naatan · 2024-10-01T21:22:55Z

pkg/checkoutinfo/checkoutinfo.go

+// Owner returns the project owner from activestate.yaml.
+// Note: cannot read this from buildscript because it may not exist yet.
+func (c *CheckoutInfo) Owner() string {
+	return c.project.Owner()


Please update all the project methods as legacy, same as we did with the commitID. That extends to the Set..() ones also.

You said in the previous PR that not everything should go into the checkoutinfo package. If we want to work with projects, we use the project package. If we want to work with buildscripts, we use the buildscript package. By making everything legacy, we're going to be forcing ourselves into buildscripts so that we end up with prime.buildscript.Owner(), etc. instead of prime.project.Owner(), etc.

That said, I have serious reservations about this. Not only is it going to balloon the PR by 4x, but I can see us walking back on this and undoing it is going to be really messy. We already walked back the CommitID() -> LegacyCommitID() transition. (That is, we went from project.CommitID() to localcommit.Get() and back to project.LegacyCommitID() via localcommit.Get().) It's a mess.

As I've been working on this PR, I've been thinking that if build script parse fails, we have no clue which project we're working on. state reset LOCAL cannot fix it because we need to parse it to get the project info. Since build scripts are far more brittle than activestate.yaml, this will probably happen pretty frequently as users make edits, or as build scripts change format over time.

As an example, start with a v46 build script and try to do something with this PR. It'll fail with a parse error. There's a lot we'll have to do to make it more robust, especially if we end up priming the primer with a buildscript instead of a projectfile.

Finally, my editor doesn't have method refactoring, so I'd either have to do this by hand or figure out how to use a new editor that has such functionality. If you really want this, can we make it a new ticket? :)

Let's chat on Slack when you're around to figure out the approach. I totally feel you on this starting to feel messy. It's worth having another look at our use-case and seeing if this is in fact the right solution, before we commit to it.

The refactor itself isn't much of an issue here though, with tooling that's really easy to do. I'm more worrying about if this is actually going to save us pain during the transition from as.yaml > buildscripts, rather than cause pain.

Naatan · 2024-10-01T21:28:17Z

pkg/platform/model/buildplanner/buildscript.go

+	host := constants.DefaultAPIHost
+	if hostOverride := os.Getenv(constants.APIHostEnvVarName); hostOverride != "" {
+		host = hostOverride
+	}
+	pjf := projectfile.NewProjectField()
+	err := pjf.LoadProject(fmt.Sprintf("https://%s/%s/%s", host, owner, project))


Nit: might be nice to add the host stuff directly to the NewProjectField() constructor. Then just use the setters, and you don't need LoadProject here at all.

…nfigurable. That way the package doesn't depend on config.

Raw contains the unprocessed checkout info string.

mitchell-as · 2024-10-04T17:01:20Z

Per conversation on Zoom, closing this for a less invasive solution.

mitchell-as added 3 commits September 26, 2024 17:40

Participle should unquote strings.

a71d906

Initial support for commit info in buildscript.

7e2bdf1

Of the form: ``` Project: URL Time: time ```

Add warning about using an outdated buildscript.

59bbc83

mitchell-as added the Test: all label Sep 26, 2024

Replace localcommit with new primed checkoutinfo package.

1504f9b

mitchell-as force-pushed the mitchell/dx-3024-2 branch 2 times, most recently from fb8182c to b19f652 Compare September 30, 2024 16:29

mitchell-as added Test: buildscripts and removed Test: all labels Sep 30, 2024

mitchell-as force-pushed the mitchell/dx-3024-2 branch from 2b5df01 to e8add31 Compare September 30, 2024 19:22

mitchell-as added 4 commits September 30, 2024 15:38

Include branch in buildscript checkout info.

232c25f

Project owner, name, and branch mutations should go through checkouti…

b1bac0a

…nfo. Centralize this so the buildscript and projectfile can remain in sync.

Synchronize build script when changing checkout info like commitID.

d773f01

Use TIME instead of at_time in build scripts.

9f5ffc9

mitchell-as force-pushed the mitchell/dx-3024-2 branch from e8add31 to 9f5ffc9 Compare September 30, 2024 19:38

mitchell-as requested a review from Naatan September 30, 2024 19:48

mitchell-as marked this pull request as ready for review September 30, 2024 19:48

Naatan requested changes Oct 1, 2024

View reviewed changes

mitchell-as added 4 commits October 2, 2024 05:38

Do not try to infer a branch from a project and commit ID.

168c37c

Checkoutinfo should accept an optin buildscript flag rather than a co…

375a26f

…nfigurable. That way the package doesn't depend on config.

Make checkout info a part of BuildScript, not BuildScript.raw.

39f6f36

Raw contains the unprocessed checkout info string.

Mark checkoutinfo parse errors as input errors, not external errors.

f7eb3ab

mitchell-as added Test: all Test: buildscripts and removed Test: buildscripts Test: all labels Oct 2, 2024

mitchell-as closed this Oct 2, 2024

mitchell-as reopened this Oct 2, 2024

mitchell-as requested a review from Naatan October 2, 2024 12:50

mitchell-as closed this Oct 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add separate checkout info block to buildscripts. #3514

Add separate checkout info block to buildscripts. #3514

mitchell-as commented Sep 26, 2024 •

edited by github-actions bot

Loading

Naatan left a comment

Naatan Oct 1, 2024

mitchell-as Oct 2, 2024

Naatan Oct 1, 2024

Naatan Oct 1, 2024

Naatan Oct 1, 2024

Naatan Oct 1, 2024

Naatan Oct 1, 2024

Naatan Oct 1, 2024

Naatan Oct 1, 2024

mitchell-as Oct 2, 2024

Naatan Oct 2, 2024

Naatan Oct 1, 2024

mitchell-as commented Oct 4, 2024

Add separate checkout info block to buildscripts. #3514

Add separate checkout info block to buildscripts. #3514

Conversation

mitchell-as commented Sep 26, 2024 • edited by github-actions bot Loading

Naatan 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

mitchell-as commented Oct 4, 2024

mitchell-as commented Sep 26, 2024 •

edited by github-actions bot

Loading