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

Set title/body from commit-message if given #3576

Closed

Conversation

pbrisbin
Copy link

@pbrisbin pbrisbin commented Dec 18, 2024

Hi there-

I'd like to set just one input to customize the commit-message, title, and body
without having to duplicate things. So here is a PR that I think does that with
a pretty simple implementation scoped to inputs parsing.

Apologies if I've missed conventions or details anywhere, just let me know.

I also have a few tangential questions that came up while doing this:

  1. Requiring ../lib/utils in the tests (vs just ../utils) meant I had to run a build any time I changed something for the tests to see the change. This seems inconvenient, am I missing something?
  2. The type of body and bodyPath are string (not string | null), so I'm using the === '' condition to determine if they've not been set. However, a few lines below I see you were doing if (inputs.bodyPath). I would think that condition can never be false if bodyPath is only ever a string -- but it seems to work? I'm not a JavaScript expert, so I may be missing something here too. Should I be doing !inputs.body (etc) in my own conditions?

And here is the commit message of my change with further details:

If title, body, and body-path are all not given, and
commit-message is, parse the title and body from the
'commit-message'.

A properly formatted message (title, blank line, then body) will be used
as title and body. Otherwise, the first line is used as title and the
remaining lines as body. Another valid approach could be to take all the
lines as title, or all the lines until the first blank as title, but the
former is unlikely to be the user's intention and the latter would
introduce complexity.

The parsing function was added in utils to facilitate testing.

If `title`, `body`, and `body-path` are all not given, and
`commit-message` is, parse the `title` and `body` from the
'commit-message'.

A properly formatted message (title, blank line, then body) will be used
as title and body. Otherwise, the first line is used as title and the
remaining lines as body. Another valid approach could be to take all the
lines as title, or all the lines until the first blank as title, but the
former is unlikely to be the user's intention and the latter would
introduce complexity.

The parsing function was added in `utils` to facilitate testing.
@pbrisbin pbrisbin marked this pull request as ready for review December 18, 2024 13:09
@peter-evans
Copy link
Owner

Hi @pbrisbin

I'd like to set just one input to customize the commit-message, title, and body without having to duplicate things.

Are you saying the reason for proposing this feature is that you just don't want to duplicate information in the yaml config? Or do you have some workflow where you already have a commit with a message?

I like to keep this action as simple as possible, and I don't really see a good reason to include this feature. This extra complexity just to save a couple of duplicated lines in the yaml workflow isn't worth it I think.

@pbrisbin
Copy link
Author

pbrisbin commented Dec 18, 2024

that you just don't want to duplicate information in the yaml config?

That one. As one example, we have automation that bumps LTS resolver and then opens a PR. We like to give it a nice title/body in commit and PR:

- uses: ...
  with:
    commit-message: |
      Bump resolver in backend/stack.yaml

      This updates resolver from X to Y, you can find comparison
      details at ...
    title: "Bump resolver in backend/stack.yaml"
    body: |
      This updates resolver from X to Y, you can find comparison
      details at ...

We have a lot of automation that creates PRs like this, and they are all doing that, matching the commit + PR details.

If I were to open such a PR locally, I would just put the details in the commit and use gh pr create --fill. I thought it might be reasonable to expect an action like this to support similar things as gh pr create.

In my motivating case, it's actually a bit worse than that, because that bump automation exposes a (single) commit-message output that we're using. This actually makes it more difficult to do from the outside.

- uses: ...
  with:
    commit-message: ${{ steps.bump.outputs.commit-message }}
    title: # have to get first line out of bump.outputs.commit-message somehow
    body: # have to get all-but-first-line out of bump.outputs.commit-message somehow

Of course, it's possible. We could use a prep step to parse things into new outputs, or even extend that bump automation action to output more things itself. But I thought the complexity added within this action was pretty minimal, so a better trade-off than doing any of those things ourselves (multiplied by however many other folks do similar things).

But if you disagree it's a better trade, then fair enough.

@pbrisbin
Copy link
Author

I wonder, would it be acceptably simple to go the other way? If commit-message is not given, default it to {title}\n\n{body}. I probably would've opened this PR doing that, if I had thought of it. I was too wrapped up in matching the behavior of gh pr create --fill.

@peter-evans
Copy link
Owner

It's a bit tricky to change defaults because some of them are hardcoded in the action.yml config.

My feeling at the moment is that this behaviour should be added as an additional step before the action runs, as you've suggested. I don't think it's a common problem for users, and this is the first time anyone has suggested such a feature. If I get more feedback about this issue in future I will revisit it.

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