-
Notifications
You must be signed in to change notification settings - Fork 308
Conversation
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.
Overall, I think that suggesting an approach such as the example below is ugly from a syntax point of view; and also sub-optimal, since a separate step is added to just define a parameter:
- name: Read CHANGELOG
id: changelog
run: |
echo "::set-output name=body::$(cat CHANGELOG.md)
However, I find this example very useful in use cases where the content of CHANGELOG.md
is generated dynamically. In such cases, using ::set-output name=body::
allows to avoid writing data to disk.
Therefore, I suggest to:
- Enhance this PR to also accept a filename as the
body
parameter, and add an example. - Add some additional commands to the example above. E.g., extracting a list of commits from git.
If it would be preferred to send nothing in the request for the body attribute I could instead compose the parameters with an optional body attribute
I think it is preferred.
@@ -12,6 +12,7 @@ For more information on these inputs, see the [API Documentation](https://develo | |||
|
|||
- `tag_name`: The name of the tag for this release | |||
- `release_name`: The name of the release | |||
- `body`: Text describing the contents of the release |
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 think that the body
is expected to contain some description, not content.
Related to the comment about supporting a filename, this description might left as: Description of the release.
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.
That copy was taken from the Create Release documentation from the API
Create Release Input
Name Type Description body string Text describing the contents of the tag.
I felt consistency was important, but don't have strong feelings. @IAmHughes do you have a preference?
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 took as a reference that the placeholder when editing a release is 'Describe this release'. I think it is more accurate than 'Text describing the contents of the *'.
@@ -26,6 +27,7 @@ async function run() { | |||
repo, | |||
tag_name: tag, | |||
name: releaseName, | |||
body, |
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.
As commented in #4 (comment), I think it'd be desirable to support providing a filename too:
body: ((body.slice(0,5) == 'file:') ? createReadStream(body.slice(6)) : body),
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.
That is a clever feature, and I could see how it would be handy. But feels like it is out of the scope for this pull request. Mostly because the test coverage and error handling for this would be more involved.
Would you like to propose it as a follow up to these changes? I have a little concern about a magic string for the body, but will defer to @IAmHughes.
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 can open a PR against csexton:add-body
(in your fork). When merged, this PR will be automatically updated.
Regarding the magic string, it's only a placeholder. I think it'd be better to use something like body: !my_filename.md
, so that quotes are not required.
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'm personally not a fan of a magic string prefix either, since it makes assumptions about the actual content of the body.
If anything, I'd prefer having a separate param like body_from_file
(or whatever) and having it error if both are present, however I also feel like it could be handled as a separate PR.
@@ -49,6 +50,10 @@ jobs: | |||
with: | |||
tag_name: ${{ github.ref }} | |||
release_name: Release ${{ github.ref }} | |||
body: | |
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 add another example with e.g. body: changelog.md
or body: "file: changelog.md"
(depending on how it is implemented).
Fixes actions#4 This adds the ability to include the body parameter when creating the release. - name: Checkout code uses: actions/checkout@master - name: Create Release id: create_release uses: actions/create-release@master with: tag_name: ${{ github.ref }} release_name: Release ${{ github.ref }} body: Release body This also supports a multiline body: - name: Create Release id: create_release uses: actions/create-release@master with: tag_name: ${{ github.ref }} release_name: Release ${{ github.ref }} body: | This is a multiline body with more than one line Or if you want the contents of a file: - name: Read CHANGELOG id: changelog run: | echo "::set-output name=body::$(cat CHANGELOG.md)" - name: Create Release id: create_release uses: actions/create-release@master with: release_name: Release ${{ github.ref }} body: ${{ steps.changelog.outputs.body }} One decision I made in favor of less code was to send an empty body when there was non present. If it would be preferred to send nothing in the request for the `body` attribute I could instead compose the parameters with an optional body attribute: releaseParams = { owner, repo, tag_name: tag, name: releaseName, draft, prerelease }; if (body) releaseParams.body = body; const createReleaseResponse = await github.repos.createRelease( releaseParams );
.... we need this! |
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.
Looks good to me, thanks for the PR. I can see the benefit of including the option of passing in a body parameter. This body could also feasibly be dynamically generated via an upstream action, and then exposed as an output of that action / consumed as an input by create-release
👍
OK so, "technically" this solution works except So here is an ACTUAL solution with a multi-line CHANGELOG that can be passed as a variable for anyone beating their head against the wall like I have been. The trick being, you have to escape all new line characters and carriage returns before passing them into set-output.
|
Fixes #4
This adds the ability to include the body parameter when creating the release.
This also supports a multiline body:
Or if you want the contents of a file (or use a script to generate changelog):
One decision I made in favor of less code was to send an empty body when there was non present. If it would be preferred to send nothing in the request for the
body
attribute I could instead compose the parameters with an optional body attribute:If this is preferred let me know and I can update the PR.