[no-release-notes] add dolt ci init and destroy #8517

coffeegoddd · 2024-11-01T22:23:11Z

This PR adds hidden commands for creating and destroying Dolt CI tables, which will be used for continuous integration on DoltHub/DoltLab.

Currently, when the hidden dolt ci commands are run, they will create/destroy all dolt ci tables and create a new dolt commit. However, this commit does not produce a diff.

These CI tables are currently alterable, so user's can change their contents and schema. Ideally, these would only be alterable/writeable by dolt itself, but we have no such interface for that.

Currently, the method CreateDoltCITables uses the approach of creating the tables against the root value. I didn't think to pass the queryist.Query method down until I created DestroyDoltCITables, so it's possible that's the preferred approach?

coffeegoddd · 2024-11-01T23:08:42Z

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`0157405`	ok	5937457

version	total_tests
`0157405`	5937457

correctness_percentage
100.0

fulghum

Looks good. Nothing major, but some suggestions for additional tests as well as a few things I think would help tidy up the changes or make it easier for future code readers.

I didn't think much about the specific table structure/design – I think I remember that being reviewed in a doc a while back. Let me know if you are looking for feedback on that, too, and I'm happy to spend a little time thinking about it.

fulghum · 2024-11-01T23:07:52Z

integration-tests/bats/ci.bats

+    run dolt sql -q "select * from dolt_ci_workflows;"
+    [ "$status" -eq 0 ]


(minor) If you only want to test that a command exits cleanly, without an error code, you can omit the run command just execute the dolt command directly. If it returns an error status, then the test will fail, without you needing to explicitly test the $status var. Not a big deal, but could help shrink down this function a lot.

fulghum · 2024-11-01T23:20:35Z

go/cmd/dolt/commands/ci/destroy.go

+	}
+
+	var verr errhand.VerboseError
+	err = schema.DestroyDoltCITables(sqlCtx, db, queryist.Query, name, email)


The package name schema seems a little too broad/general; when first reading it, I didn't know it was specific to dolt CI. This is likely to be confusing for future code readers since we already have widely used schema package referenced in the code, so I'd pick a different name, preferably something specific to dolt CI.

One ideas is to move the contents of dolt_ci/schema into dolt_ci and use that as the package name. It looks like dolt_ci is currently empty, except for the schema subdirectory, so that would simplify the directory structure and make the package name more explicit/obvious for code readers to quickly understand.

fulghum · 2024-11-01T23:27:16Z

integration-tests/bats/ci.bats

+     skip "session ctx in shell is not the same as session in server"
+    fi
+}
+


Might be worth adding a test for the case where dolt ci init is called on a database that has already been initialized, and check for the dolt ci has already been initialized error message.

fulghum · 2024-11-01T23:29:39Z

go/libraries/doltcore/doltdb/system_table.go

+// HasDoltCIPrefix returns a boolean whether or not the provided string is prefixed with the DoltCINamespace. Users should
+// not be able to create tables in this reserved namespace.


Another good, quick test would be to try and create a user table with the dolt_ci_ prefix and assert that it results in an error.

fulghum · 2024-11-01T23:34:49Z

go/libraries/doltcore/doltdb/system_table.go

+		// todo: find way to make these writable by the dolt process
+		// todo: but not by user


I haven't dug into this, but my gut says adding these tables to the writable system tables list is going to make it hard to prevent users from writing to them. It seems like leaving them out of here and having another code path that writes to them might be what you need to achieve this.

Not a big deal for the initial version of this, just brainstorming ideas here for future iterations.

fulghum · 2024-11-04T17:55:42Z

go/libraries/doltcore/sqle/enginetest/dolt_ci_queries.go

+	{
+		Name: "select * from dolt_ci_workflows;",
+		SetUpScript: []string{
+			"insert into dolt_ci_workflows values" +


Looks like these new Go tests are failing in CI because the dolt_ci_workflows table doesn't exist yet. Perhaps the RunDoltCIConfigTests function below needs to call the schema create action function that the CLI uses to initialize these tables first?

fulghum · 2024-11-04T18:23:33Z

go/libraries/doltcore/env/actions/dolt_ci/schema/schema.go

+	"github.com/dolthub/dolt/go/store/datas"
+)
+
+var ExpectedDoltCITablesOrdered = []doltdb.TableName{


Looks like several exported vars/functions probably don't need to be exported and would be better to keep them encapsulated in this namespace to prevent them from showing up as options for use outside this package. For example, ExpectedDoltCITablesOrderd could be unexported since it's only used in this file.

I'll tag a few others I noticed in this file, but would be good to take a quick check through and lowercase as much as we can.

fulghum · 2024-11-04T18:25:20Z

go/libraries/doltcore/env/actions/dolt_ci/schema/schema.go

+	doltdb.TableName{Name: doltdb.WorkflowSavedQueryStepExpectedRowColumnResultsTableName},
+}
+
+type QueryFunc func(ctx *sql.Context, query string) (sql.Schema, sql.RowIter, *sql.QueryFlags, error)


QueryFunc seems like it could be changed to queryFunc since it's not used outside of this package. That'll help prevent this from coming up as an option for other people, too, since it shouldn't be used anywhere else it seems like.

Also... perhaps less so with un-exported functions/types/etc, but I generally think if you're taking the time to define a type, you should help out future code readers and give it a quick doc string to document intent.

fulghum · 2024-11-04T18:44:33Z

go/libraries/doltcore/env/actions/dolt_ci/schema/schema.go

+}
+
+func commitCIDestroy(ctx *sql.Context, queryFunc QueryFunc, commiterName, commiterEmail string) error {
+	return sqlWriteQuery(ctx, queryFunc, fmt.Sprintf("CALL DOLT_COMMIT('-Am' 'Successfully destroyed Dolt CI', '--author', '%s <%s>');", commiterName, commiterEmail))


This is potentially a little sneaky – if a user has other outstanding changes to their tables or even new tables they've just created, then the -A flag will automatically stage all of them and include them in the commit. This should probably be careful to only stage the affected CI tables. This would be another good test to add, too – both for the init and destroy cases.

In the init code path below in CreateDoltCITables, it looks like this problem doesn't exist – it only stages the Dolt CI tables, so it won't add any user changes that happen to be in the workspace.

fulghum · 2024-11-04T18:49:12Z

go/libraries/doltcore/env/actions/dolt_ci/schema/schema.go

+
+var ExpectedDoltCITablesOrdered = []doltdb.TableName{
+	doltdb.TableName{Name: doltdb.WorkflowsTableName},
+	doltdb.TableName{Name: doltdb.WorkflowEventsTableName},


(minor) since this is already in a slice of type doltdb.TableName you can omit the type name on these lines and make the code a little easier to read:

Suggested change

doltdb.TableName{Name: doltdb.WorkflowEventsTableName},

{Name: doltdb.WorkflowEventsTableName},

coffeegoddd force-pushed the db/dolt-ci-init branch from 00506fe to 1ac8b47 Compare November 1, 2024 22:29

coffeegoddd requested review from jycor and fulghum and removed request for jycor November 1, 2024 22:31

/{go,integration-tests}: add hidden dolt ci init and destroy commands

0157405

coffeegoddd force-pushed the db/dolt-ci-init branch from 1ac8b47 to 0157405 Compare November 1, 2024 22:40

coffeegoddd added the correctness_approved label Nov 1, 2024

fulghum approved these changes Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[no-release-notes] add dolt ci init and destroy #8517

[no-release-notes] add dolt ci init and destroy #8517

coffeegoddd commented Nov 1, 2024 •

edited

Loading

coffeegoddd commented Nov 1, 2024

fulghum left a comment

fulghum Nov 1, 2024

fulghum Nov 1, 2024

fulghum Nov 1, 2024

fulghum Nov 1, 2024

fulghum Nov 1, 2024

fulghum Nov 4, 2024

fulghum Nov 4, 2024

fulghum Nov 4, 2024

fulghum Nov 4, 2024

fulghum Nov 4, 2024

		run dolt sql -q "select * from dolt_ci_workflows;"
		[ "$status" -eq 0 ]

		// HasDoltCIPrefix returns a boolean whether or not the provided string is prefixed with the DoltCINamespace. Users should
		// not be able to create tables in this reserved namespace.

		// todo: find way to make these writable by the dolt process
		// todo: but not by user

	doltdb.TableName{Name: doltdb.WorkflowEventsTableName},
	{Name: doltdb.WorkflowEventsTableName},

[no-release-notes] add dolt ci init and destroy #8517

Are you sure you want to change the base?

[no-release-notes] add dolt ci init and destroy #8517

Conversation

coffeegoddd commented Nov 1, 2024 • edited Loading

coffeegoddd commented Nov 1, 2024

fulghum 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

coffeegoddd commented Nov 1, 2024 •

edited

Loading