spindle: move the clone step out of nixery into a shared package for all spindle engines #3114

Merged

evan.jarrett.net 11mo ago

targeting master

Merged opened by and assigned to

evan.jarrett.net 11mo ago targeting master

This pull request has no versions submitted yet.

Comments

oppi.li 11mo ago

thanks for the contribution! couple of nits:

we already have a top level workflow package that also defines CloneOpts (to unmarshal the workflow yaml file however), it is a bit confusing to introduce another workflow package inside spindle with a similar struct CloneOptions (it also shares some of the same fields)
IMO a simpler route here would be to have a generic CloneStep method that is calculated from tangled.Pipeline_Workflow and returns a struct that follows the models.Step interface

evan.jarrett.net 11mo ago

@oppi.li What package would you want this generic cloneStep in?

I think your solution is better, there was no precedent for a custom models.Step outside of nixery, so I was originally trying to avoid that.

I would still eventually want RepoURL and CommitSHA exposed for my usecase. but I might have to just submit a different PR for system-level env vars.

evan.jarrett.net 11mo ago

Not entirely sure why this says there are merge conflicts when i rebased master This is an attempt to go off of your design with returning a whole step struct. I think it turns out essentially the same code... so maybe i'm missing something obvious.

oppi.li 11mo ago

that is a bit strange, i don't see anything off with the logs on the knotserver itself. that aside, some comments on the code itself:

don't think we need to cd workspaceDir, we already have this upon contrainer creation (the WorkingDir is set):

	resp, err := e.docker.ContainerCreate(ctx, &container.Config{
		Image:      addl.image,
		Cmd:        []string{"cat"},
		OpenStdin:  true, // so cat stays alive :3
		Tty:        false,
		Hostname:   "spindle",
		WorkingDir: workspaceDir,

the rest of the clone code itself looks good to me!
could you squash the two commits into one? the commit message could be spindle: introduce common clone step or similar

anirudh.fi 11mo ago

Gave it a once-over; looks good! And thanks for the tests.