Use composite action for issue creation #131
Conversation
|
You may want to also add I have looked into it myself for issue creation with |
Sorry, not sure I follow. |
No.
What is the use of the comment? The author of
|
|
Gotcha. If possible I'd like to keep the number of dependencies to other Github actions at a minimum. I'd love to have the possibility to append a new comment to an existing issue. However this can be added Also, we should release this as part of version 2.0 of the extension to avoid any breaking changes. |
| description: "summary output file path" | ||
| default: "lychee/out.md" | ||
| description: "Summary output file path" | ||
| default: "summary.md" |
There was a problem hiding this comment.
Since the working directory for this action is likely a git repo, and other steps in the job (using this composite as a single step) may also process content at this location. You should not write into it and instead prefer using a temporary location?
| default: "summary.md" | |
| default: "${{ runner.temp }}/summary.md" |
https://docs.github.com/en/actions/learn-github-actions/contexts#runner-context
entrypoint.sh could also do the same (and maybe mkdir a sub-directory for lychee?).
| required: false | ||
| fail: | ||
| description: "fail entire pipeline on error (i.e. when lychee exit code is not 0)" | ||
| description: "Fail entire pipeline on error (i.e. when lychee exit code is not 0)" |
There was a problem hiding this comment.
fail should be dropped. As should exit_code output (may still be relevant, see other review comment for step conditional).
| default: 0.10.0 | ||
| required: false | ||
| createIssue: | ||
| description: "Create Github issue if link check fails" | ||
| default: true |
There was a problem hiding this comment.
This should be opt-in.
I would not want issues being created by default when using this to lint PRs.
The only time it makes sense is for a scheduled recurring job to notify maintainers. When that is desired it should be looked up and opt-in, not occur by default which would be surprising for an action that supports other use-cases. Additionally it'd likely fail on PRs (from third-party contributors, due to running in an insecure context required permissions would be missing).
| default: true | |
| default: false |
| issueTitle: | ||
| description: "Title of Github issue which will be created if link check fails" | ||
| default: Link Checker Report | ||
| required: false | ||
| issueLabels: | ||
| description: "Attach labels to Github issue" | ||
| default: links, report, automated issue | ||
| required: false | ||
| issueRepository: | ||
| description: 'The target GitHub repository for creating issues' | ||
| default: ${{ github.repository }} | ||
| issueNumber: | ||
| description: 'The issue number of an existing issue to update' |
There was a problem hiding this comment.
It might be worthwhile for maintainers (beyond yourself) to grok this composite action easier if the scope of inputs is less vague.
A simple comment should help with that since these are all sub-inputs to the create-issue-from-file action and you're just forwarding them:
+ # These inputs only valid when `createIssue: true`
issueTitle:
description: "Title of Github issue which will be created if link check fails"
default: Link Checker Report
required: false
issueLabels:
description: "Attach labels to Github issue"
default: links, report, automated issue
required: false
issueRepository:
description: 'The target GitHub repository for creating issues'
default: ${{ github.repository }}
issueNumber:
description: 'The issue number of an existing issue to update'| required: false | ||
| issueLabels: | ||
| description: "Attach labels to Github issue" | ||
| default: links, report, automated issue |
There was a problem hiding this comment.
Personally, I'd rather not have an action add a bunch of labels polluting a projects own labels (a project I maintain has plenty already and these would be unwanted noise in the way of manual triage).
Especially if the action is not even using the labels for any purpose... that should be opt-in (no default, show the feature off in examples instead).
If you added the other action which can lookup an issue number by label (picks the issue result sorted by last updated), then something like this might be useful:
| default: links, report, automated issue | |
| default: ci-lychee |
Otherwise avoid default labels, let the user choose labels that are relevant to their project for this feature.
| @@ -46,6 +65,15 @@ runs: | |||
| INPUT_JOBSUMMARY: ${{ inputs.JOBSUMMARY }} | |||
| shell: bash | |||
|
|
|||
| - if: inputs.CREATEISSUE && steps.lychee.outputs.exit_code != 0 | |||
There was a problem hiding this comment.
It would be better to check failure status and step outcome, rather than exit_code as was discussed here:
| - if: inputs.CREATEISSUE && steps.lychee.outputs.exit_code != 0 | |
| - if: ${{ failure() && steps.lychee.outcome == 'failure' && inputs.createIssue }} |
Remember this would involve lychee step dropping inputs.fail (doesn't serve a purpose) and outputs.exit_code (redundant?).
In saying that, exit_code does communicate two kind of failures:
1: Generic failure.2: One or more links failed when checked.
And your issue creation is probably only interested in exit_code == 2 so that it has content worth reporting (Although perhaps you'd want to update/close the issue if a link failure was temporary and exit_code was 0 in a future run).
Presently you handle a related scenario that exit_code == 2 does not:
Lines 20 to 22 in 76ab977
Ideally an output to communicate the link failure is more than exit_code == 2, so that the next action reacting to that has actual content to put into the body too.. This kind of fallback content probably isn't what you'd want to see? (unless this error prevents returning the exit_code of 2?)
`continue-on-error: true` (_I need to verify some information_)
Alternatively with steps.lychee.continue-on-error: true, the job would not exit early. I'm not sure how that applies in composite action however.
I need to check if failure() is still valid with that setting, or if only lychee.outcome == 'failure' should be checked due to lychee.conclusion == 'success' always being the case.
| - if: inputs.CREATEISSUE && steps.lychee.outputs.exit_code != 0 | |
| - if: ${{ steps.lychee.outcome == 'failure' && steps.lychee.outputs.exit_code == 2 && inputs.createIssue }} |
steps.lychee.outcome == 'failure' would be redundant then, if steps.lychee.outputs.exit_code == 2 was required 😅
Finally with consideration for other failures that could occur in the action.. (_I need to verify some information_)
You may want the workflow run to fail by not using continue-on-error: true. AFAIK workflow failures will send a notification to maintainers to be aware of... Thus exit_code == 2 should not fail the action step? (but retain an output to distinguish from checks with no failed links)
Bringing us closer to the condition you have used:
| - if: inputs.CREATEISSUE && steps.lychee.outputs.exit_code != 0 | |
| - if: ${{ steps.lychee.outputs.exit_code == 2 && inputs.createIssue }} |
I think, a workflow that has other steps involved unrelated to link checking should be in separate jobs, where they would not be affected and a failed status is fine then.
It's then the decision of if running create-issue-from-file is:
- A response to handling a failed action step (link errors) creating the issue as an automated reaction/post-step. While additionally maintainers would be notified of the failure (I get emails) and the UI for the action showing a red X fail status symbol (action overview for scheduled jobs, or commit history if not on the PR check-status-suite).
- Just another valid path/outcome for the link checking action/job. Thus no notification of job failure sent out, and the job is marked with a green tick as successful since it didn't actually fail when failed links were encountered.
Personally, if I'm having an issue opened for failed links, the job worked correctly, the job was not a failure (the step itself can be), and I'd rather the job didn't exit and notify me of such.
However for a PR, no issue is needed, and failed links is actionable as either a failed job (lint), or having another step take that information and do something with it on the PR (eg: adding/updating a comment, similar to this issue support). That decision can be handled with continue-on-error: true at the expense of not knowing of real failures?
| - run: ${{ github.action_path }}/entrypoint.sh | ||
|
|
||
| - id: lychee | ||
| run: ${{ github.action_path }}/entrypoint.sh | ||
| env: | ||
| #https://github.com/actions/runner/issues/665 |
There was a problem hiding this comment.
This appears to apply to the entire env block? The comment should be above env: line then, and perhaps reference the comment too? I assume it was this one.
| - run: ${{ github.action_path }}/entrypoint.sh | ||
|
|
||
| - id: lychee | ||
| run: ${{ github.action_path }}/entrypoint.sh | ||
| env: | ||
| #https://github.com/actions/runner/issues/665 | ||
| INPUT_GITHUB_TOKEN: ${{ inputs.GITHUB_TOKEN }} |
There was a problem hiding this comment.
What is the point of this ENV? You do not appear to use INPUT_GITHUB_TOKEN in your entrypoint.sh?
| required: false | ||
| issueTitle: | ||
| description: "Title of Github issue which will be created if link check fails" | ||
| default: Link Checker Report |
There was a problem hiding this comment.
May be better to scope the issue with a prefix that indicates in the title it's a CI issue, and more specific hint of the content (since while it does contain a report, it's only being reported when failures are caught?):
| default: Link Checker Report | |
| default: 'ci(lint): Some monitored links are failing' |
| Issue creation can optionally be disabled | ||
|
|
||
| ```yaml | ||
| - name: Link Checker | ||
| uses: lycheeverse/[email protected] | ||
| with: | ||
| createIssue: false |
There was a problem hiding this comment.
| Issue creation can optionally be disabled | |
| ```yaml | |
| - name: Link Checker | |
| uses: lycheeverse/[email protected] | |
| with: | |
| createIssue: false | |
| When failing links are found, `lychee-action` can create and update an issue with the `createIssue` input: | |
| ```yaml | |
| - name: Link Checker | |
| uses: lycheeverse/[email protected] | |
| with: | |
| createIssue: true |
You might want to show off / mention other supported features too?
I'm still curious what the purpose for that would be, vs replacing the automated issue body. It's easy enough to do regardless, I have a workflow for PRs that adds a comment to the PR with a link to a deployment preview of docs. Whenever the PR is updated, the new deployment preview link is updated in that comment (the action can alternatively delete the old comment and create a new one).
Wouldn't it be better to avoid the composite action entirely then? Or offer that separately from
Personally after providing that review for you, my opinion is this is not worth maintaining compared to having better documentation. Is there a strong demand for the composited action? Provide an We know of some common examples to cater for already:
Sure, but I really don't think issue creation should be opt-out. |
How do you plan for the issue number feature to be used? Currently it seems like:
Both of those seem awkward..? Whereas with Assuming
If you did not visit the link I provided, this is what it'd look like (ignoring the composited action approach in this PR): name: 'Manually trigger action (for testing)'
on: workflow_dispatch
jobs:
lint-links-in-docs:
name: 'Lint - Documentation Links (with issue reporting)'
runs-on: ubuntu-22.04
permissions:
contents: read
issues: write
steps:
# Permissions (contents: read)
- uses: actions/checkout@v3
- name: 'Check links in docs'
id: lychee
uses: lycheeverse/[email protected]
continue-on-error: true
with:
args: --verbose --no-progress -- docs/**/*
output: ${{ runner.temp }}/lychee-report.md
# Below manages issue creation/updating when failing links are found:
# Permissions (issues: read)
- name: 'Check for an existing lychee-action issue'
id: last-issue
uses: micalevisk/[email protected]
# NOTE: This env won't be necessary soon:
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
# Find the last updated open issue with a `ci-lint-links` label:
with:
state: open
labels: ci-lint-links
# Permissions (issues: write)
- name: 'Create a new issue, or update an existing one'
if: ${{ steps.lychee.outputs.exit_code == 2 }}
uses: peter-evans/create-issue-from-file@v4
with:
title: 'ci(lint): Docs have failing links'
content-filepath: ${{ runner.temp }}/lychee-report.md
# If issue_number is empty (no match found), create a new issue, otherwise update this matched issue:
issue-number: ${{ steps.last-issue.outputs.issue_number }}
# Add a label unique enough to search for, and any other labels relevant to maintainers:
labels: ci-lint-links, docs, bugWell.. that would work, except To verify, just swap out those last two steps of the example to view the available outputs. It will show that it's empty even when output - name: 'List lychee outputs'
run: |
echo '::echo::on'
echo -e "lychee.outputs:\n${{ toJSON(steps.lychee.outputs) }}" |
|
Thanks a lot for the excellent review. I will go through the comments once I find some time but from a cursory look all suggestions make sense. You are right in that coupling issue creation with this action is probably not the way to go. I've changed my mind. Instead, let's work on the examples folder as you suggested. |
👍
I can contribute these, and refactor In exchange, could you please redirect your time on resolving the |
|
Please do, that would be awesome!
Sounds good. |
@polarathene do you still plan to work on the example folder? The exit code should be propagated correctly now. |
Yes, sorry been rather busy lately 😅
👍 |
|
I got lost in the conversation here. |
Any particular examples you're after? I'm unfortunately swamped right now, contributing examples is still on my todo list but I have deadlines to meet 😅 |
I want to automatically create an issue once errors are found with lychee-action. My first impression was that it should work already, one just needs to know the correct configuration which there are no examples of yet. Hence my question.
Great to hear that you still plan on working on it! 👍 |
Open an issue and mention me in it, it can track the request for that example.
I believe I had something similar to what you're asking for when I was working on a similar workflow. I didn't manage to complete it as it relied on some other actions to update, that should be taken care of now. I hope you don't mind the delay, but I should be able to find time to sort that out for you this month. If I can get on top of things this week, it might be possible to have a workflow example available this weekend 👍 We decided that This is a copy/paste from a workflow I shared elsewhere which should be able to be revised to meet your requirements. This was while working on a PR for If I remember correctly, this would create an issue when So this just needs to consider a successful run of name: 'Testing - Manually trigger action'
on:
workflow_dispatch:
inputs:
exit-code:
type: number
description: Override the EXIT_CODE
kebab-bool:
type: boolean
description: Test kebab-case conditions
default: true
jobs:
lint-links-in-docs:
name: 'Lint - Documentation Links (with issue reporting)'
runs-on: ubuntu-22.04
permissions:
contents: read
issues: write
steps:
- uses: actions/checkout@v3
- id: link-reporter
run: |
echo '**report content here**' > "${{ runner.temp }}/report.md"
# Only set when an explicit value is given to workflow input,
EXIT_CODE=${{ github.event.inputs.exit-code }}
# Otherwise fallback to 0:
[[ -z ${EXIT_CODE} ]] && EXIT_CODE=0
# Disabling this bool will set `EXIT_CODE=2`
[[ ${{ github.event.inputs.kebab-bool }} == 'false' ]] && EXIT_CODE=2
echo '::echo::on'
echo "EXIT_CODE: ${EXIT_CODE}"
# Test kebab-case output condition:
echo "::set-output name=exit-code::${EXIT_CODE}"
# Below manages issue creation/updating when failing links are found:
- name: 'Check for an existing link-reporter issue'
id: last-issue
uses: micalevisk/[email protected]
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
state: open
labels: ci-lint-links
- name: 'Create a new issue, or update an existing one'
if: ${{ steps.link-reporter.outputs.exit-code == 2 }}
uses: peter-evans/create-issue-from-file@v4
with:
title: 'ci(lint): Docs have failing links'
content-filepath: ${{ runner.temp }}/report.md
issue-number: ${{ steps.last-issue.outputs.issue_number }}
labels: ci-lint-links, bug |
No description provided.