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.

Sign up for GitHub

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

Use composite action for issue creation #131

Closed
wants to merge 9 commits into from
Closed

Use composite action for issue creation #131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5886c61
Use composite action for issue creation
mre May 30, 2022
70a220f
Update README.md
mre Jul 1, 2022
66e9a66
Parallelize tests
mre Jul 1, 2022
274942d
Ignore failing host for now
mre Jul 1, 2022
1ace38e
create issue
mre Jul 1, 2022
97aa116
Test create local issue
mre Jul 1, 2022
14eb799
Update action.yml
mre Jul 3, 2022
981f565
Update main.yml
mre Jul 3, 2022
99e8691
Update README.md
mre Jul 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions .github/workflows/links.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,7 @@ jobs:
uses: ./ # Uses an action in the root directory
with:
args: --user-agent "Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0" --verbose --exclude spinroot.com --no-progress './**/*.md' './**/*.html'
issueTitle: Link Checker Report
issueLabels: report, automated issue
env:
GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}

- name: Create Issue From File
uses: peter-evans/create-issue-from-file@v4
with:
title: Link Checker Report
content-filepath: ./lychee/out.md
labels: report, automated issue
73 changes: 57 additions & 16 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,44 +5,85 @@ on:
- pull_request

jobs:
lychee-action:
defaults:
runs-on: ubuntu-latest
continue-on-error: true
name: Test the lychee link checker action
name: Test default action usage
steps:
# To use this repository's private action,
# we must check out the repository
- name: Checkout
uses: actions/checkout@2541b1294d2704b0964813337f33b291d3f8596b
- name: test defaults
uses: ./
- uses: ./
with:
fail: true
- name: test explicit lychee version
uses: ./

explicit:
runs-on: ubuntu-latest
name: Test explicit lychee version
steps:
- name: Checkout
uses: actions/checkout@2541b1294d2704b0964813337f33b291d3f8596b
- uses: ./
with:
lycheeVersion: 0.9.0
- name: test globs
uses: ./

globs:
runs-on: ubuntu-latest
name: Test globs
steps:
- name: Checkout
uses: actions/checkout@2541b1294d2704b0964813337f33b291d3f8596b
- uses: ./
with:
args: --exclude-mail --verbose --no-progress './**/*.md' './**/*.html'
fail: true
- name: test workflow inputs
uses: ./
fail: true

workflow-inputs:
runs-on: ubuntu-latest
name: Test workflow inputs
steps:
- name: Checkout
uses: actions/checkout@2541b1294d2704b0964813337f33b291d3f8596b
- uses: ./
with:
args: -v fixtures/TEST.md
format: json
output: /tmp/foo.json
fail: true
- name: directory
uses: ./

directory:
runs-on: ubuntu-latest
name: Test directory
steps:
- name: Checkout
uses: actions/checkout@2541b1294d2704b0964813337f33b291d3f8596b
- uses: ./
with:
args: --exclude-mail .
fail: true
- name: test format override
uses: ./

format-override:
runs-on: ubuntu-latest
name: Test format override
steps:
- name: Checkout
uses: actions/checkout@2541b1294d2704b0964813337f33b291d3f8596b
- uses: ./
with:
args: --format markdown -v fixtures/TEST.md
format: doesnotexist # gets ignored if format set in args
output: /tmp/foo.txt
fail: true

create-issue:
runs-on: ubuntu-latest
name: Test create issue
steps:
- name: Checkout
uses: actions/checkout@2541b1294d2704b0964813337f33b291d3f8596b
- uses: ./
with:
args: http://www.deadlinkcity.com --include 'e=400'
issueRepository: lycheeverse/lychee-action
issueNumber: 141
issueLabels: test, some, labels
1 change: 1 addition & 0 deletions .lycheeignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
lycheeverse/lychee-action@.*
https://github.com/org/repo
https://francoisbest.com
27 changes: 13 additions & 14 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@

Quickly check links in Markdown, HTML, and text files using [lychee].

When used in conjunction with [Create Issue From
File](https://github.com/peter-evans/create-issue-from-file), issues will be
created when the action finds link problems.

## Usage

Here is a full example of a GitHub workflow file:
Expand All @@ -32,18 +28,19 @@ jobs:
- uses: actions/checkout@v3

- name: Link Checker
id: lychee
uses: lycheeverse/[email protected]
uses: lycheeverse/[email protected]
env:
GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}

- name: Create Issue From File
if: steps.lychee.outputs.exit_code != 0
uses: peter-evans/create-issue-from-file@v3
with:
title: Link Checker Report
content-filepath: ./lychee/out.md
labels: report, automated issue
```

Issue creation can optionally be disabled

```yaml
- name: Link Checker
uses: lycheeverse/[email protected]
with:
createIssue: false
Comment on lines +37 to +43
Copy link

Choose a reason for hiding this comment

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

Suggested change
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?

```

### Alternative approach:
Expand Down Expand Up @@ -181,6 +178,8 @@ This action is based on
lychee (written in Rust) instead of liche (written in Go) for link checking. For
a comparison of both tools, check out this [comparison
table](https://github.com/lycheeverse/lychee#features).
We also use [create-issue-from-file](https://github.com/peter-evans/create-issue-from-file)
to create issues.

## License

Expand All @@ -196,4 +195,4 @@ at your option.
[lychee-args]: https://github.com/lycheeverse/lychee#commandline-parameters
[troubleshooting]: https://github.com/lycheeverse/lychee/blob/master/docs/TROUBLESHOOTING.md
[security]: https://francoisbest.com/posts/2020/the-security-of-github-actions
[dependabot]: https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file
[dependabot]: https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file
46 changes: 37 additions & 9 deletions action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,36 +6,55 @@ inputs:
default: "--verbose --no-progress './**/*.md' './**/*.html'"
required: false
format:
description: "summary output format (e.g. json)"
description: "Summary output format (e.g. json)"
default: "markdown"
required: false
output:
description: "summary output file path"
default: "lychee/out.md"
description: "Summary output file path"
default: "summary.md"
Copy link

Choose a reason for hiding this comment

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

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?

Suggested change
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)"
Copy link

Choose a reason for hiding this comment

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

fail should be dropped. As should exit_code output (may still be relevant, see other review comment for step conditional).

#85 (comment)

default: false
required: false
jobSummary:
description: "write Github job summary at the end of the job (written on Markdown output only)"
description: "Write Github job summary at the end of the job (written on Markdown output only)"
default: true
required: false
lycheeVersion:
description: "use custom version of lychee link checker"
description: "Use custom version of lychee link checker"
default: 0.10.0
required: false
createIssue:
description: "Create Github issue if link check fails"
default: true
Copy link

Choose a reason for hiding this comment

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

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).

Suggested change
default: true
default: false

required: false
issueTitle:
description: "Title of Github issue which will be created if link check fails"
default: Link Checker Report
Copy link

Choose a reason for hiding this comment

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

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?):

Suggested change
default: Link Checker Report
default: 'ci(lint): Some monitored links are failing'

required: false
issueLabels:
description: "Attach labels to Github issue"
default: links, report, automated issue
Copy link

Choose a reason for hiding this comment

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

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:

Suggested change
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.

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'
Comment on lines +32 to +44
Copy link

Choose a reason for hiding this comment

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

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'

Comment on lines +40 to +44
Copy link

Choose a reason for hiding this comment

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

These two inputs are missing required field, but according to GHA docs the field is mandatory?

Suggested change
issueRepository:
description: 'The target GitHub repository for creating issues'
default: ${{ github.repository }}
issueNumber:
description: 'The issue number of an existing issue to update'
issueRepository:
description: 'The target GitHub repository for creating issues'
default: ${{ github.repository }}
required: false
issueNumber:
description: 'The issue number of an existing issue to update'
required: false


runs:
using: "composite"
steps:
- name: install lychee
run: |
- run: |
Copy link

Choose a reason for hiding this comment

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

Why was the step name dropped?

curl -LO 'https://github.com/lycheeverse/lychee/releases/download/v${{ inputs.LYCHEEVERSION }}/lychee-v${{ inputs.LYCHEEVERSION }}-x86_64-unknown-linux-gnu.tar.gz'
tar -xvzf lychee-v${{ inputs.LYCHEEVERSION }}-x86_64-unknown-linux-gnu.tar.gz
chmod 755 lychee
mv lychee /usr/local/bin/lychee
shell: bash
- run: ${{ github.action_path }}/entrypoint.sh

- id: lychee
run: ${{ github.action_path }}/entrypoint.sh
env:
#https://github.com/actions/runner/issues/665
Copy link

Choose a reason for hiding this comment

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

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.

INPUT_GITHUB_TOKEN: ${{ inputs.GITHUB_TOKEN }}
Copy link

Choose a reason for hiding this comment

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

What is the point of this ENV? You do not appear to use INPUT_GITHUB_TOKEN in your entrypoint.sh?

Expand All @@ -46,6 +65,15 @@ runs:
INPUT_JOBSUMMARY: ${{ inputs.JOBSUMMARY }}
shell: bash

- if: inputs.CREATEISSUE && steps.lychee.outputs.exit_code != 0
Copy link

Choose a reason for hiding this comment

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

It would be better to check failure status and step outcome, rather than exit_code as was discussed here:

Suggested change
- 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:

lychee-action/entrypoint.sh

Lines 20 to 22 in 76ab977

if [ ! -f "${LYCHEE_TMP}" ]; then
echo "No output. Check pipeline run to see if lychee panicked." > "${LYCHEE_TMP}"
fi

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.

Suggested change
- 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:

Suggested change
- 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?

uses: peter-evans/create-issue-from-file@v4
with:
title: ${{ inputs.ISSUETITLE }}
labels: ${{ inputs.ISSUELABELS }}
repository: ${{ inputs.ISSUEREPOSITORY }}
issue-number: ${{ inputs.ISSUENUMBER }}
content-filepath: ${{ inputs.OUTPUT }}
Comment on lines +68 to +75
Copy link

Choose a reason for hiding this comment

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

Does the inputs.<UPPERCASE FORM> here actually work? Referencing the GHA docs:

When you specify an input in a workflow file or use a default input value, GitHub creates an environment variable for the input with the name INPUT_<VARIABLE_NAME>. The environment variable created converts input names to uppercase letters and replaces spaces with _ characters.

If the action is written using a composite, then it will not automatically get INPUT_<VARIABLE_NAME>. If the conversion doesn't occur, you can change these inputs manually.

And related docs for composite action step.with:

Optional A map of the input parameters defined by the action. Each input parameter is a key/value pair.
Input parameters are set as environment variables.
The variable is prefixed with INPUT_ and converted to upper case.

Both are about ENV creation of inputs, I'm not sure why you're referencing the inputs upper-case here?

Here you are using the inputs context, not ENV.

Just pointing this out as I'm not sure what you're referencing to take that approach.

Related note regarding differences with action output handling for composite actions

If you need to provide outputs from composite action steps, keep in mind the syntax is a bit different:

Optional outputs use the same parameters as outputs.<output_id> and outputs.<output_id>.description
but also includes the value token.

outputs.<output_id>.value
Required The value that the output parameter will be mapped to. You can set this to a string or an expression with context.
For example, you can use the steps context to set the value of an output to the output value of a step.

Example from docs:

outputs:
  random-number:
    description: "Random number"
    value: ${{ steps.random-number-generator.outputs.random-id }}
runs:
  using: "composite"
  steps:
    - id: random-number-generator
      run: echo "::set-output name=random-id::$(echo $RANDOM)"
      shell: bash


branding:
icon: "external-link"
color: "purple"