Monorepo for Tangled tangled.org

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

merged opened by evan.jarrett.net targeting master from evan.jarrett.net/core: spindle-clone
Labels
refactor
assignee

None yet.

Participants 3
AT URI
at://did:plc:pddp4xt5lgnv2qsegbzzs4xg/sh.tangled.repo.pull/3m5xkfp2rcn22
+27 -31
Interdiff #1 โ†’ #2
+4 -2
appview/db/pipeline.go
··· 168 168 169 169 // this is a mega query, but the most useful one: 170 170 // get N pipelines, for each one get the latest status of its N workflows 171 + func GetPipelineStatuses(e Execer, limit int, filters ...filter) ([]models.Pipeline, error) { 171 - func GetPipelineStatuses(e Execer, filters ...filter) ([]models.Pipeline, error) { 172 172 var conditions []string 173 173 var args []any 174 174 for _, filter := range filters { ··· 205 205 join 206 206 triggers t ON p.trigger_id = t.id 207 207 %s 208 + order by p.created desc 209 + limit %d 210 + `, whereClause, limit) 208 - `, whereClause) 209 211 210 212 rows, err := e.Query(query, args...) 211 213 if err != nil {
+1 -1
appview/pages/templates/repo/compare/compare.html
··· 17 17 {{ end }} 18 18 19 19 {{ define "mainLayout" }} 20 + <div class="px-1 flex-grow col-span-full flex flex-col gap-4"> 20 - <div class="px-1 col-span-full flex flex-col gap-4"> 21 21 {{ block "contentLayout" . }} 22 22 {{ block "content" . }}{{ end }} 23 23 {{ end }}
+1 -1
appview/pages/templates/repo/settings/general.html
··· 58 58 {{ i "loader-circle" "w-4 h-4 animate-spin hidden group-[.htmx-request]:inline" }} 59 59 </button> 60 60 </div> 61 + </fieldset> 61 - <fieldset> 62 62 </form> 63 63 {{ end }} 64 64
-1
appview/pages/templates/user/fragments/editBio.html
··· 31 31 class="py-1 px-1 w-full" 32 32 name="pronouns" 33 33 placeholder="they/them" 34 - pattern="[a-zA-Z]{1,6}[\/\s\-][a-zA-Z]{1,6}" 35 34 value="{{ $pronouns }}" 36 35 > 37 36 </div>
+3
appview/pipelines/pipelines.go
··· 82 82 83 83 ps, err := db.GetPipelineStatuses( 84 84 p.db, 85 + 30, 85 86 db.FilterEq("repo_owner", repoInfo.OwnerDid), 86 87 db.FilterEq("repo_name", repoInfo.Name), 87 88 db.FilterEq("knot", repoInfo.Knot), ··· 124 125 125 126 ps, err := db.GetPipelineStatuses( 126 127 p.db, 128 + 1, 127 129 db.FilterEq("repo_owner", repoInfo.OwnerDid), 128 130 db.FilterEq("repo_name", repoInfo.Name), 129 131 db.FilterEq("knot", repoInfo.Knot), ··· 193 195 194 196 ps, err := db.GetPipelineStatuses( 195 197 p.db, 198 + 1, 196 199 db.FilterEq("repo_owner", repoInfo.OwnerDid), 197 200 db.FilterEq("repo_name", repoInfo.Name), 198 201 db.FilterEq("knot", repoInfo.Knot),
+2
appview/pulls/pulls.go
··· 178 178 179 179 ps, err := db.GetPipelineStatuses( 180 180 s.db, 181 + len(shas), 181 182 db.FilterEq("repo_owner", repoInfo.OwnerDid), 182 183 db.FilterEq("repo_name", repoInfo.Name), 183 184 db.FilterEq("knot", repoInfo.Knot), ··· 648 649 repoInfo := f.RepoInfo(user) 649 650 ps, err := db.GetPipelineStatuses( 650 651 s.db, 652 + len(shas), 651 653 db.FilterEq("repo_owner", repoInfo.OwnerDid), 652 654 db.FilterEq("repo_name", repoInfo.Name), 653 655 db.FilterEq("knot", repoInfo.Knot),
+14 -10
appview/repo/compare.go
··· 116 116 } 117 117 118 118 // if user is navigating to one of 119 + // /compare/{base}...{head} 119 120 // /compare/{base}/{head} 121 + var base, head string 122 + rest := chi.URLParam(r, "*") 123 + 124 + var parts []string 125 + if strings.Contains(rest, "...") { 126 + parts = strings.SplitN(rest, "...", 2) 127 + } else if strings.Contains(rest, "/") { 128 + parts = strings.SplitN(rest, "/", 2) 129 + } 130 + 131 + if len(parts) == 2 { 132 + base = parts[0] 133 + head = parts[1] 120 - // /compare/{base}...{head} 121 - base := chi.URLParam(r, "base") 122 - head := chi.URLParam(r, "head") 123 - if base == "" && head == "" { 124 - rest := chi.URLParam(r, "*") // master...feature/xyz 125 - parts := strings.SplitN(rest, "...", 2) 126 - if len(parts) == 2 { 127 - base = parts[0] 128 - head = parts[1] 129 - } 130 134 } 131 135 132 136 base, _ = url.PathUnescape(base)
+1 -14
appview/repo/repo_util.go
··· 1 1 package repo 2 2 3 3 import ( 4 - "crypto/rand" 5 - "math/big" 6 4 "slices" 7 5 "sort" 8 6 "strings" ··· 90 88 return 91 89 } 92 90 93 - func randomString(n int) string { 94 - const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" 95 - result := make([]byte, n) 96 - 97 - for i := 0; i < n; i++ { 98 - n, _ := rand.Int(rand.Reader, big.NewInt(int64(len(letters)))) 99 - result[i] = letters[n.Int64()] 100 - } 101 - 102 - return string(result) 103 - } 104 - 105 91 // grab pipelines from DB and munge that into a hashmap with commit sha as key 106 92 // 107 93 // golang is so blessed that it requires 35 lines of imperative code for this ··· 118 104 119 105 ps, err := db.GetPipelineStatuses( 120 106 d, 107 + len(shas), 121 108 db.FilterEq("repo_owner", repoInfo.OwnerDid), 122 109 db.FilterEq("repo_name", repoInfo.Name), 123 110 db.FilterEq("knot", repoInfo.Knot),
-1
appview/repo/router.go
··· 61 61 // for example: 62 62 // /compare/master...some/feature 63 63 // /compare/master...example.com:another/feature <- this is a fork 64 - r.Get("/{base}/{head}", rp.Compare) 65 64 r.Get("/*", rp.Compare) 66 65 }) 67 66
+1 -1
nix/pkgs/knot-unwrapped.nix
··· 4 4 sqlite-lib, 5 5 src, 6 6 }: let 7 + version = "1.11.0-alpha"; 7 - version = "1.9.1-alpha"; 8 8 in 9 9 buildGoApplication { 10 10 pname = "knot";
spindle/engines/nixery/engine.go

This file has not been changed.

spindle/engines/nixery/setup_steps.go

This file has not been changed.

spindle/models/clone.go

This file has not been changed.

spindle/models/clone_test.go

This file has not been changed.

History

4 rounds 5 comments
sign up or login to add to the discussion
1 commit
expand
spindle: implement shared clone step outside of nixery engine.
expand 1 comment

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

pull request successfully merged
2 commits
expand
spindle: move the clone step out of nixery into a shared function for all spindle engines
Create a BuildCloneStep function that returns a step struct that inherits from models.Step
expand 2 comments

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.

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
2 commits
expand
spindle: move the clone step out of nixery into a shared function for all spindle engines
Create a BuildCloneStep function that returns a step struct that inherits from models.Step
expand 0 comments
1 commit
expand
spindle: move the clone step out of nixery into a shared function for all spindle engines
expand 2 comments

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

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