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

Link Checking #315

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions .github/workflows/checking.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
name: Link Checking

on:
schedule:
- cron: 0 0 * * * # At 00:00 (UTC)
septs marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Nit picks: this could say "daily".

Copy link
Member Author

Choose a reason for hiding this comment

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

This hides the key information

  1. Run time
  2. Timezone

Copy link
Member

Choose a reason for hiding this comment

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

I mean in addition to what you already have.

Copy link
Member

Choose a reason for hiding this comment

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

@septs Does that info matter, though? Your choices of time and timezone were both arbitrary, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

This issue need a guideline.

Copy link
Contributor

@codehag codehag Oct 15, 2020

Choose a reason for hiding this comment

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

You are right, its a bit deeper in there:

Github actions do not support cron aliases: https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#scheduled-events

So you are right, this has to be 0 0 * * *. We can resolve this issue.

Copy link
Contributor

@codehag codehag Oct 15, 2020

Choose a reason for hiding this comment

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

Maybe add a comment to that effect in case anyone tries.

Something to the effect of Github actions does not support cron aliases should be enough

Copy link
Member Author

Choose a reason for hiding this comment

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

i think, this is unpredictable random event. is this really what we need?

Copy link
Contributor

@codehag codehag Oct 15, 2020

Choose a reason for hiding this comment

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

What you have looks good -- we can't use daily as per the documentation you linked. We just need the comment, because people are likely to get confused (as I did) about why we aren't using daily. Someone might try to change it as a "fix", and might end up inadvertently breaking the cron job. We don't want that. We just want to get across the intention of this configuration. If it needs to change later, we can do that later.

Outside of github actions, sometimes you want to override cron jobs, so daily acts like a variable that you can set to a specific time for multiple scripts if necessary. It is also more readable, and unless there is an override it usually means 0 0 * * *. Unfortunately, we can't use that here but that is fine.

Copy link
Member Author

@septs septs Oct 15, 2020

Choose a reason for hiding this comment

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

We maybe need use GitHub Support.


jobs:
link-checking:
name: Link Checking
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: JSCIG/link-checker-action@v1
Copy link
Member

Choose a reason for hiding this comment

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

This should be a hash, not a tag or branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

tag based is the general practice of github action

Copy link
Member

Choose a reason for hiding this comment

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

That makes no difference to me. If we need enhancements in the future, we can change the hash.

Copy link
Member Author

Choose a reason for hiding this comment

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

for any github action, use hash based?

Copy link
Member

Choose a reason for hiding this comment

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

That is my preference, yes. I can make exceptions for GitHub-owned actions (github.com/actions/*).

Copy link
Member Author

@septs septs Sep 29, 2020

Choose a reason for hiding this comment

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

i choose general community practice

unless i have must be a reason to this practice. (e.q: write in https://github.com/tc39/how-we-work)

Copy link
Member

Choose a reason for hiding this comment

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

@septs this isn't a "PR author choose" kind of thing; there's security/reliability concerns.

Copy link
Member Author

Choose a reason for hiding this comment

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

This issue need a guideline.

@ljharb and @michaelficarra can write to how-we-work ?

Copy link
Member Author

Choose a reason for hiding this comment

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

i want to see tc39 github actions practice guideline.

Copy link
Member

Choose a reason for hiding this comment

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

The guideline is "whatever the repo maintainer chooses", generally - I'm not sure why this is a sticking point.