Skip to content
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

Refactor PR Body regexing into independent GH Action #159

Closed
wants to merge 2 commits into from

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Aug 9, 2024

Changes

  • Introduces pr-body-parse composite GitHub Action to parse key/value from body of PR triggering CI

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16 rmartin16 force-pushed the refactor-pr-body-regex branch 13 times, most recently from afdf11f to b81583c Compare August 9, 2024 22:04
@rmartin16
Copy link
Member Author

Everything takes longer than you expect...

Couple of notes:

  • Both app-create and install-briefcase check out their own versions of beeware/.github
    • This is to ensure there is a reliable location to run pr-body-parse from without requiring callers to checkout beeware/.github to a specific directory
    • Alternatively, pr-body-parse could be run from the main branch....but that makes updating the Action harder in the future....and we've also already spent a lot of time talking about this more general situation...
  • Handling for github.token
    • I'm assuming that the GitHub Token will be handled the same as before....at least insofar as it seemed to be working correctly before and it should still be now
  • printf shananigans
    • Anywhere the value from the PR body might be printed, I tried to ensure printf was used....just to prevent any chance of command injection...

@rmartin16 rmartin16 marked this pull request as ready for review August 9, 2024 22:39
@freakboy3742
Copy link
Member

I presume this was part of the discussion around making the template branch configurable; if we're not pursuing that as an idea (which I think is where we landed?) is this refactor still worth it?

@rmartin16
Copy link
Member Author

I think it's probably still worthwhile in case we do find more things we want to parse out of the PR body.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could have other uses for PR parsing... although I'm having difficulty thinking what other variables we might need to extract.

Conceptually, I'm on board with the idea of factoring out common behaviours. My concern is that Actions syntax is so verbose that a refactor like this, that sounds like a good idea on paper, ends up with more code to yield a "simplification". I'm mildly concerned that the config is sufficiently complex that it's not necessarily obvious how to drive it; adding more complexity is potentially only going to make that worse.

If we do go ahead - a couple of clarifications and suggestions inline.

.github/actions/pr-body-parse/action.yml Show resolved Hide resolved
.github/actions/install-briefcase/action.yml Show resolved Hide resolved
@rmartin16
Copy link
Member Author

Conceptually, I'm on board with the idea of factoring out common behaviours. My concern is that Actions syntax is so verbose that a refactor like this, that sounds like a good idea on paper, ends up with more code to yield a "simplification". I'm mildly concerned that the config is sufficiently complex that it's not necessarily obvious how to drive it; adding more complexity is potentially only going to make that worse.

Yeah...I am also a bit disappointed with its verbosity. Since, though, I've been thinking that given our use-case is always running two regexes to get the git repo and ref, I could just allow the Action to accept two regexes and output two corresponding values (although, since we can't use YAML lists it would probably require regex-1 & regex-2 for inputs and value-1 and value-2 for outputs).

@rmartin16
Copy link
Member Author

Since I didn't explicitly solicit any feedback in my previous post :) what do you think about updating the Action to accept multiple regexes? Do you think that would improve the situation/implementation?

@freakboy3742
Copy link
Member

I guess it might be a little shorter, but I'm not sure it makes it shorter by enough to make the drop in functionality worth it. If we're doing this refactor, it's because we forsee a need to extract other variables from the PR body; limiting ourself to a fixed number of regexes seems like an odd choice to make under those circumstances.

The code that is here looks fine to me; I guess my question to you is whether you see it actually being useful in the longer term. I'm not 100% sure I do... but that said, you've written the PR, and we might use it... I've marked it as approved; but I'll leave it in your court as to whether we actually land it, or close it out as an interesting experiment but ultimately not worth it.

@rmartin16
Copy link
Member Author

ehh....we don't have a use for it now (or even a foreseeable one) and neither of us really like the refactor's outcome. we can come back to it if we need to.

@rmartin16 rmartin16 closed this Aug 17, 2024
@rmartin16 rmartin16 deleted the refactor-pr-body-regex branch August 17, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants