-
Notifications
You must be signed in to change notification settings - Fork 4
add megalinter and update dependencies #204
base: develop
Are you sure you want to change the base?
Conversation
grmbyrn
commented
Jun 4, 2024
- Add MegaLinter
- Update:
- TypeScript
- Prettier
- Sass
- Shiki
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.
Hi @grmbyrn the PR looks good but there are a couple of suggestions:
Update dependencies
This could be done in an independent PR unless you need to update them for a feature included in the PR. It's OK to do it in the same PR if you are sure the PR will be merged soon and it's not complex.
Tagline
I think now it doesn't make sense to use 2 lines:
Maybe we can make the min width longer:
What do you think?
Megalinter
I think we should enable Megalinter and fix megalinter errors in the same PR otherwise we are going to have a failing workflow until we fix all errors.
You can run megalinter locally with:
npx mega-linter-runner
MegaLinter generates a local text report so you can fix all of them.
You should use the latest MegaLinter: https://github.com/oxsecurity/megalinter/releases/tag/v7.12.0
.github/workflows/deploy.yml
Outdated
environment: | ||
name: github-pages | ||
url: ${{ steps.deployment.outputs.page_url }} | ||
|
||
steps: |
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.
Hi @grmbyrn why did you remove the environment?
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.
Hi @josecelano github-pages
was giving me this error:
Value 'github-pages' is not valid
The name of the environment used by the job.
Available expression contexts: github, inputs, vars, needs, strategy, matrix
I asked ChatGPT for advice and it recommended removing the environment as it's not necessary for deploying to GitHub Pages. It said that if you want to specify an environment, you can define it separately and reference it within your workflow.
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.
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.
Okay, so do you recommend putting the environment back and ignoring the error?
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.
Hi @josecelano I've been looking for a solution to this and found an issue on GitHub Actions repo from May about including github-pages
as a valid option. It's currently been placed in the backlog of projects. Do you think it's okay to put it back in and ignore the error?
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.
Hi @josecelano I've been looking for a solution to this and found an issue on GitHub Actions repo from May about including
github-pages
as a valid option. It's currently been placed in the backlog of projects. Do you think it's okay to put it back in and ignore the error?
Yes, @graeme, I would put it back, and I would open a new issue to inform other contributors they could have that error until that schema is updated. We can close it when it is updated, since I think we can't do anything else.
Hi @josecelano okay, I'll remember to dependencies separately unless necessary.
Yes, I agree that this looks better on one line and can fix that.
Okay, should I fix megalinter errors in this PR? |
Hi @grmbyrn yes, if you like. But I will probably take you long. It's usually hard to fix all the MegaLinter errors if it's not installed from the first commit. I would open a new PR just for that. |
Okay, I'll open a new PR for the MegaLinter errors. Is there anything else to fix before merging this PR? The environment in |
Add back in removed environment. Error being shown raised in GitHub Actions with this issue: actions/deploy-pages#344