Add separate checkout info block to buildscripts. #3545

mitchell-as · 2024-10-17T17:17:56Z

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.
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 ```

mitchell-as · 2024-10-17T18:41:57Z

pkg/buildscript/buildscript.go

+		return false, errs.Wrap(err, "Unable to clone other buildscript")
+	}
+
+	// Do not compare project URLs.


If we compare URLs (which include commit IDs), we'll more often than not get false "your buildscript has changes, please commit" errors. Just compare the build expression and at time bits of the build script.

mitchell-as · 2024-10-17T19:16:43Z

🎵 Churn baby churn 🎵

Naatan

Minimal churn!

Naatan · 2024-10-18T17:06:53Z

internal/runbits/buildscript/file.go

+	if url, err := projectURL(proj, commitId.String()); err == nil {
+		script.SetProject(url)
+	} else {
+		return errs.Wrap(err, "Unable to set project")
+	}


You should be able to use project.URL() here.

No, I cannot because the projecter has the old URL (with the old commit ID) from before a mutation. localcommit.Set() updates the URL on disk, but the primed project was not reloaded/reset/whatever, so its URL remains the same as it was originally.

I don't really see the point in changing this because when we replace project files with build scripts, we'll have to do something like constructing this URL again. I thought the point of this runbit was to know as little about projects as possible.

Good point. I don't like how opinionated the current solution is wrt the formatting of the URL though. I guess what this is touching on is that we probably need some central mechanic for project URLs. Still, to avoid further churn I think for this story we can maybe have a middle ground; like use project.URL() and only splice in the commitID. So you'd avoid having any opinion on the URL beyond the commitID parameter.

Naatan · 2024-10-18T17:07:30Z

internal/runbits/buildscript/file.go

+	url, err := projectURL(proj, commitID.String())
+	if err != nil {
+		return errs.Wrap(err, "Could not construct project URL")
+	}


Same as above; use project.URL().

Same as above.

Naatan · 2024-10-18T17:11:13Z

pkg/buildscript/raw.go

 	"github.com/brunoga/deep"
 )

 // Tagged fields will be filled in by Participle.
 type rawBuildScript struct {
+	Info        *string       `parser:"(RawString @RawString RawString)?"`


I think we should separate this from the parser entirely. Because we want to be able to access project info even if the buildscript itself has parsing issues.

That sounds like a task for another day when we replace project files with build scripts, because right now, callers of buildscript.Unmarshal() expect either a full, populated build script, or an error explaining why it didn't get one. We would have to refactor all callers to handle receiving a build script with only checkout info, or a fully populated one. Even still, I don't think they could do anything meaningful with a script that only has checkout info. (When we replace project files with build scripts, I can see us having the primer or some really early function make use of this to set things up, but then perhaps if that's the only use, it should be in a separate function from buildscript.Unmarshal(), which can keep its existing behavior.)

Regardless, I feel like this is a rabbit hole and we shouldn't prescribe something that's not currently needed since we don't know the future requirements yet.

Ok, that's fair. We'll leave it for when we get rid of the project URL in activestate.yaml.

Naatan · 2024-10-18T20:14:49Z

internal/runbits/buildscript/file.go

+	if url, err := projectURL(proj, commitId.String()); err == nil {
+		script.SetProject(url)
+	} else {
+		return errs.Wrap(err, "Unable to set project")
+	}


Good point. I don't like how opinionated the current solution is wrt the formatting of the URL though. I guess what this is touching on is that we probably need some central mechanic for project URLs. Still, to avoid further churn I think for this story we can maybe have a middle ground; like use project.URL() and only splice in the commitID. So you'd avoid having any opinion on the URL beyond the commitID parameter.

Naatan · 2024-10-18T20:16:14Z

pkg/buildscript/raw.go

 	"github.com/brunoga/deep"
 )

 // Tagged fields will be filled in by Participle.
 type rawBuildScript struct {
+	Info        *string       `parser:"(RawString @RawString RawString)?"`


Ok, that's fair. We'll leave it for when we get rid of the project URL in activestate.yaml.

Also, ensure an updated build script with commit ID is always written.

Naatan · 2024-10-18T21:19:55Z

👍

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

mitchell-as added 5 commits October 17, 2024 14:14

Participle should unquote strings.

4fedcbd

Initial support for commit info in buildscript.

2d34115

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

Add warning about using an outdated buildscript.

ba3aec7

Use TIME instead of at_time in build scripts.

1acb72d

Attempt to fix failing test.

6430b81

mitchell-as removed the Test: all label Oct 17, 2024

mitchell-as force-pushed the mitchell/dx-3024-4 branch from 09f2396 to 6430b81 Compare October 17, 2024 18:37

mitchell-as added the Test: buildscripts label Oct 17, 2024

mitchell-as commented Oct 17, 2024

View reviewed changes

mitchell-as closed this Oct 17, 2024

mitchell-as reopened this Oct 17, 2024

mitchell-as requested a review from Naatan October 17, 2024 19:16

mitchell-as marked this pull request as ready for review October 17, 2024 19:16

Naatan requested changes Oct 18, 2024

View reviewed changes

mitchell-as requested a review from Naatan October 18, 2024 18:26

Naatan requested changes Oct 18, 2024

View reviewed changes

Leverage project's URL instead of constructing a full one.

34781f0

Also, ensure an updated build script with commit ID is always written.

mitchell-as added Test: commit and removed Test: commit labels Oct 18, 2024

mitchell-as closed this Oct 18, 2024

mitchell-as reopened this Oct 18, 2024

mitchell-as requested a review from Naatan October 18, 2024 21:04

Naatan approved these changes Oct 18, 2024

View reviewed changes

mitchell-as merged commit 4feb03e into version/0-47-0-RC1 Oct 18, 2024
16 of 21 checks passed

mitchell-as deleted the mitchell/dx-3024-4 branch October 18, 2024 21:27

as-builds added a commit that referenced this pull request Oct 18, 2024

Merge branch version/0-47-0-RC1 to adopt changes from PR #3545

5bbb1c9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add separate checkout info block to buildscripts. #3545

Add separate checkout info block to buildscripts. #3545

mitchell-as commented Oct 17, 2024 •

edited by github-actions bot

Loading

mitchell-as Oct 17, 2024

mitchell-as commented Oct 17, 2024

Naatan left a comment

Naatan Oct 18, 2024

mitchell-as Oct 18, 2024 •

edited

Loading

Naatan Oct 18, 2024

Naatan Oct 18, 2024

mitchell-as Oct 18, 2024

Naatan Oct 18, 2024

mitchell-as Oct 18, 2024

Naatan Oct 18, 2024

Naatan Oct 18, 2024

Naatan Oct 18, 2024

Naatan commented Oct 18, 2024

Add separate checkout info block to buildscripts. #3545

Add separate checkout info block to buildscripts. #3545

Conversation

mitchell-as commented Oct 17, 2024 • edited by github-actions bot Loading

Choose a reason for hiding this comment

mitchell-as commented Oct 17, 2024

Naatan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as Oct 18, 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

Naatan commented Oct 18, 2024

mitchell-as commented Oct 17, 2024 •

edited by github-actions bot

Loading

mitchell-as Oct 18, 2024 •

edited

Loading