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

AO3-6590 Add reviewdog for linting ERB files #4604

Merged
merged 3 commits into from
Aug 19, 2023

Conversation

Bilka2
Copy link
Contributor

@Bilka2 Bilka2 commented Aug 13, 2023

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-6590

Purpose

Use reviewdog to run ERB lint on pull requests because Hound does not run it (despite being configured for it).

Demo PR from same repo: Bilka2#1
Demo PR from forked repo: Bilka2#2 (see reviewdog's docs on Graceful degradation for PRs from forks)

Testing Instructions

See Jira.

Credit

Bilka (he/him)

@github-actions github-actions bot added Scope: Tests Only Only changes automated tests or test configuration Awaiting Review labels Aug 13, 2023
@Bilka2
Copy link
Contributor Author

Bilka2 commented Aug 13, 2023

As is, the workflow fails in this repository:

tk0miya/action-erblint@667687e is not allowed to be used in otwcode/otwarchive. Actions in this workflow must be: within a repository owned by otwcode, created by GitHub, or verified in the GitHub Marketplace.

See GitHub Actions docs on Allowing select actions and reusable workflows to run.

Bilka2 added 2 commits August 15, 2023 17:24
It doesn't work. And if that would be fixed in the future, it would be
linting twice.
@github-actions github-actions bot removed the Scope: Tests Only Only changes automated tests or test configuration label Aug 15, 2023
bundler-cache: true

- name: erb-lint
uses: tk0miya/action-erblint@667687e73b44e7b7a710a1204b180f49f80ebb5e
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't trust the creator of the action, it's usually best practices to use the hash version for actions, because it refers to a specific commit. That means that you can view the repository, read the source code of the action, and be sure that when the action runs, it's running only code that you've already reviewed.

Tags, on the other hand, can be switched to point to a different commit, which means that the owner of the action could choose to replace the existing code with some kind of exploit.

I'm not sure the extra security is necessary, in this case, but I can see both sides of the argument.

Copy link
Contributor Author

@Bilka2 Bilka2 Aug 17, 2023

Choose a reason for hiding this comment

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

GitHub recommends using a SHA in their docs, especially if the action author is not a verified creator, like in this case.

However, this workflow has very few permissions ("write checks" for PRs from the same repo, "read checks" for third party repos), so if it were compromised I don't think it could do much harm. But I'd rather be safe than sorry, because github actions and their permissions are relatively new to me.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough! Thanks for the link @Bilka2!

@brianjaustin brianjaustin added Reviewed: Ready to Merge Scope: Tests Only Only changes automated tests or test configuration and removed Awaiting Review labels Aug 18, 2023
@sarken sarken merged commit 03cafde into otwcode:master Aug 19, 2023
weeklies pushed a commit to weeklies/otwarchive that referenced this pull request Aug 19, 2023
* AO3-6590 Add reviewdog ERB lint runner

* AO3-6590 Remove erb lint from Hound

It doesn't work. And if that would be fixed in the future, it would be
linting twice.

* AO3-6590 Rename reviewdog workflow to be more generic
weeklies pushed a commit to weeklies/otwarchive that referenced this pull request Aug 20, 2023
* AO3-6590 Add reviewdog ERB lint runner

* AO3-6590 Remove erb lint from Hound

It doesn't work. And if that would be fixed in the future, it would be
linting twice.

* AO3-6590 Rename reviewdog workflow to be more generic
@Bilka2 Bilka2 deleted the AO3-6590-add-reviewdog-erblint branch August 21, 2023 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed: Ready to Merge Scope: Tests Only Only changes automated tests or test configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants