-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow running in place on an existing checkout #53
base: main
Are you sure you want to change the base?
Conversation
default: ".github/workflows" | ||
default: '[".github/workflows"]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
in_place: | ||
description: "Update the files in place" | ||
required: false | ||
default: "false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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: "" |
var repo *git.Repository | ||
var fs billy.Filesystem | ||
var githubClient *github.Client | ||
var token string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a bunch of these (particularly githubClient
and token
) don't get initialized on the "directory" path. Should we remove them?
ActionsPath: os.Getenv("INPUT_ACTIONS"), | ||
ActionsPaths: envToStrings("INPUT_ACTIONS"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -53,7 +53,6 @@ type FrizbeeAction struct { | |||
ImagesReplacer *replacer.Replacer | |||
BFS billy.Filesystem | |||
Repo *git.Repository | |||
bodyBuilder *strings.Builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -1,4 +1,4 @@ | |||
FROM golang:alpine3.19@sha256:0466223b8544fb7d4ff04748acc4d75a608234bf4e79563bff208d2060c0dd79 | |||
FROM golang:1.23.1-alpine3.20@sha256:ac67716dd016429be8d4c2c53a248d7bcdf06d34127d3dc451bda6aa5a87bc06 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we include this change in another PR? I think it's a good change.
Hello @segiddins! I'm excited about this PR -- anything we can do to help you out in revisitng this? |
I'll try to circle back next week! |
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