Allow running in place on an existing checkout #53

segiddins · 2024-09-10T22:27:52Z

Ran into this when I was setting up rubygems/rubygems.org#5019 -- I want to just run frizbee on the current checkout, so it can run on every PR

blkt · 2024-09-12T13:26:10Z

action.yml

-    default: ".github/workflows"
+    default: '[".github/workflows"]'


Hey @segiddins thank you for your contributio!
I'm not an expert here, but this looks like a breaking change, how would this impact existing users of the action?
cc @evankanderson

evankanderson

I like the idea. A couple requests:

Rather than a true/false value, let's make the in-place enablement use a path, where the empty value uses the current behavior.
It seems like the in-place behavior implicitly disables the PR path today. It feels like we should make those separate -- e.g. create the githubClient if the token is present, and pass that in to the action whether it's working on a local checkout or remote. (Unless there's a good reason to make these arguments exclusive.)

evankanderson · 2024-09-12T18:02:21Z

action.yml

+  in_place:
+    description: "Update the files in place"
+    required: false
+    default: "false"


So, I agree that the existing action is a bit weird -- what would you think about making this be a directory path argument, where the default "" value means to do the existing fetch-from-github behavior? i.e.

Suggested change

in_place:

description: "Update the files in place"

required: false

default: "false"

repo_root:

description: "Operate on files in the specified filesystem location. If unspecified, check out files from the current repo."

required: false

default: ""

evankanderson · 2024-09-12T18:04:42Z

main.go

+	var repo *git.Repository
+	var fs billy.Filesystem
+	var githubClient *github.Client
+	var token string


It looks like a bunch of these (particularly githubClient and token) don't get initialized on the "directory" path. Should we remove them?

The tricky thing is they're used when creating the action.FrizbeeAction, so they need to be defined unconditionally

evankanderson · 2024-09-12T18:06:51Z

main.go

-		ActionsPath:        os.Getenv("INPUT_ACTIONS"),
+		ActionsPaths:       envToStrings("INPUT_ACTIONS"),


I'd prefer to add a second argument which covers the multiple-paths option (which could be exclusive with the single argument), OR to change valToStrings to also interpret the input foo as equivalent to ["foo"]. (To be honest, I'd rather have had comma-delimited strings over JSON elsewhere, but that ship has sailed.)

Thinking further, I converting foo to ["foo"] is dangerous, because we might end up with ["[\"foo\",\n]" depending on how strict the JSON parser is and how lazy the user is. Maybe add a second argument for multiple GitHub Actions directories?

evankanderson · 2024-09-12T18:16:55Z

pkg/action/action.go

@@ -53,7 +53,6 @@ type FrizbeeAction struct {
 	ImagesReplacer  *replacer.Replacer
 	BFS             billy.Filesystem
 	Repo            *git.Repository
-	bodyBuilder     *strings.Builder


Removing bodyBuilder seems like a (nice) simple refactor which doesn't change behavior. I could approve that change real fast if it wasn't entangled in the behavior change around ActionsPaths changing contents.

JAORMX · 2024-09-13T07:37:55Z

Dockerfile

@@ -1,4 +1,4 @@
-FROM golang:alpine3.19@sha256:0466223b8544fb7d4ff04748acc4d75a608234bf4e79563bff208d2060c0dd79
+FROM golang:1.23.1-alpine3.20@sha256:ac67716dd016429be8d4c2c53a248d7bcdf06d34127d3dc451bda6aa5a87bc06


Can we include this change in another PR? I think it's a good change.

evankanderson · 2024-09-17T13:03:49Z

Hello @segiddins! I'm excited about this PR -- anything we can do to help you out in revisitng this?

segiddins · 2024-09-17T21:10:20Z

I'll try to circle back next week!

segiddins · 2024-09-28T17:11:53Z

@evankanderson This should be ready for another look, along with the other two PRs!

jhrozek · 2024-09-30T08:09:23Z

@segiddins hey, it looks like the linter is complaining about the cyclomatic complexity of the init function now. I'll leave it up to you if you want to reduce the complexity or just slap a nolint above (I've done both in about equal amounts...)

Signed-off-by: Samuel Giddins <[email protected]>

…omatic complexity Signed-off-by: Samuel Giddins <[email protected]>

segiddins · 2024-09-30T16:09:45Z

Let's see if this helps?

Signed-off-by: Samuel Giddins <[email protected]>

jhrozek

I think this looks good and the changes seem to be backwards-compatible

evankanderson requested review from blkt and evankanderson September 12, 2024 13:09

blkt reviewed Sep 12, 2024

View reviewed changes

evankanderson reviewed Sep 12, 2024

View reviewed changes

JAORMX reviewed Sep 13, 2024

View reviewed changes

segiddins force-pushed the segiddins/run-in-place branch from 850cb44 to 11c08b2 Compare September 23, 2024 23:48

segiddins added 5 commits September 30, 2024 09:03

Allow running in place on an existing checkout

a3db4bd

Remove unneeded check for empty paths

e0d6001

Add a second input for multiple paths

feaabc8

Signed-off-by: Samuel Giddins <[email protected]>

Revert to underscored input names

b5a8a46

Signed-off-by: Samuel Giddins <[email protected]>

Extract action path list computation to a new function to reduce cycl…

7080386

…omatic complexity Signed-off-by: Samuel Giddins <[email protected]>

segiddins force-pushed the segiddins/run-in-place branch from c162fda to 7080386 Compare September 30, 2024 16:09

golangci-lint passing

24d0658

Signed-off-by: Samuel Giddins <[email protected]>

jhrozek approved these changes Oct 7, 2024

View reviewed changes

jhrozek merged commit 4ab6687 into stacklok:main Oct 7, 2024
4 checks passed

segiddins deleted the segiddins/run-in-place branch October 7, 2024 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow running in place on an existing checkout #53

Allow running in place on an existing checkout #53

segiddins commented Sep 10, 2024

blkt Sep 12, 2024

evankanderson left a comment

evankanderson Sep 12, 2024

evankanderson Sep 12, 2024

segiddins Sep 23, 2024

evankanderson Sep 12, 2024

evankanderson Sep 12, 2024

evankanderson Sep 12, 2024

segiddins Sep 23, 2024

JAORMX Sep 13, 2024

segiddins Sep 23, 2024

evankanderson commented Sep 17, 2024

segiddins commented Sep 17, 2024

segiddins commented Sep 28, 2024

jhrozek commented Sep 30, 2024

segiddins commented Sep 30, 2024

jhrozek left a comment

		default: ".github/workflows"
		default: '[".github/workflows"]'

		ActionsPath: os.Getenv("INPUT_ACTIONS"),
		ActionsPaths: envToStrings("INPUT_ACTIONS"),

		@@ -1,4 +1,4 @@
		FROM golang:alpine3.19@sha256:0466223b8544fb7d4ff04748acc4d75a608234bf4e79563bff208d2060c0dd79
		FROM golang:1.23.1-alpine3.20@sha256:ac67716dd016429be8d4c2c53a248d7bcdf06d34127d3dc451bda6aa5a87bc06

Allow running in place on an existing checkout #53

Allow running in place on an existing checkout #53

Conversation

segiddins commented Sep 10, 2024

Choose a reason for hiding this comment

evankanderson 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

evankanderson commented Sep 17, 2024

segiddins commented Sep 17, 2024

segiddins commented Sep 28, 2024

jhrozek commented Sep 30, 2024

segiddins commented Sep 30, 2024

jhrozek left a comment

Choose a reason for hiding this comment