More GitHub actions support #943
Conversation
| const [, org, repo, , sha] = path.split('/'); | ||
|
|
||
| return `{BASE_URL}/${join(org, repo, 'tree', sha, target)}`; |
There was a problem hiding this comment.
I have another PR I'm working on for Dart or C/C++ that does this same thing. I'll make sure to move this out to a helper when I get back to it.
There was a problem hiding this comment.
I used join() for these because that's how it was done here and it seemed a little cleaner. Do you have a preference to how these are built?
There was a problem hiding this comment.
I don't mind, but I agree, your solution is more explicit.
| # @OctoLinkerResolve(https://github.com/github/codeql-action/tree/master/init) | ||
| - uses: github/codeql-action/init@v1 | ||
| # @OctoLinkerResolve(https://github.com/OctoLinker/OctoLinker) | ||
| - uses: ./ |
There was a problem hiding this comment.
For some reason this test failed but it works correctly in the browser.
✕ resolves - uses: ./ to https://github.com/OctoLinker/OctoLinker (5001 ms)
There was a problem hiding this comment.
If I navigate to https://github.com/OctoLinker/OctoLinker/blob/936dbbc793bb8e2246a1ea7b6b34c76244dd28bd/e2e/fixtures/.github/workflows/ci.yml
this line links to https://github.com/OctoLinker/OctoLinker/tree/936dbbc793bb8e2246a1ea7b6b34c76244dd28bd/
which is inline with your implementation. Maybe we need another wildcard like <root> https://github.com/OctoLinker/OctoLinker/blob/master/e2e/generate-fixtures-json.js#L64
There was a problem hiding this comment.
How do you run the e2e tests locally? I adjusted generate-fixtures-json.js to point to my fork & branch, but I keep getting the following output when trying to run them:
❯ node "c:/dev/xt0rted/OctoLinker/node_modules/jest/bin/jest.js" "c:/dev/xt0rted/OctoLinker/e2e/automated.test.js" -t "End to End tests single blob"
No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In C:\dev\xt0rted\OctoLinker
198 files checked.
testMatch: **/__tests__/**/*.[jt]s?(x), **/?(*.)+(spec|test).[tj]s?(x) - 37 matches
testPathIgnorePatterns: C:\\dev\\xt0rted\\OctoLinker\\e2e\\ - 186 matches
testRegex: - 0 matches
Pattern: c:\\dev\\xt0rted\\OctoLinker\\e2e\\automated.test.js - 0 matches
The fixtures.json was created, but the tests in automated.test.js don't run.
There was a problem hiding this comment.
There is a special jest config for e2e tests --config=e2e/jest-e2e.config.js which is used when running yarn e2e
stefanbuck
force-pushed
the
github-actions
branch
from
c02c7d7
to
361f6ec
Compare
| @@ -18,11 +18,14 @@ const async = require('async'); | |||
| // ] | |||
|
|
|||
| let username; | |||
| let sha = 'HEAD'; | |||
There was a problem hiding this comment.
This won't work when running yarn e2e local. I resist to call out the git binary to get the latest sha, because this doesn't work under all conditions for example the latest commit isn't pushed yet.
There was a problem hiding this comment.
Maybe we should consider not adding tests for relative links in workflow files. What do you think @xt0rted ?
There was a problem hiding this comment.
I'm ok with that. Should I remove that test and rebase this or do you want to merge it with the fixup commits?
If the tests weren't generated we could add a check for this one so it only ran if the github_actions env var was true.
There was a problem hiding this comment.
I think using an env var doesn't solve the actual issue, because the sha will be different all the time, right? Also this would fail when running on a fork right? Let's just remove that test and get this merged.
There was a problem hiding this comment.
When these run they pull whatever is in the current branch, so the sha will change on each run. If the test only ran when GITHUB_ACTION was true then we'd also be able to get the sha from GITHUB_SHA, but that won't work with the current test setup. Maybe in the future, but not now.
There was a problem hiding this comment.
A while ago I came across the fact that the env var GITHUB_SHA is different depending on whether your workflow runs on push or pull_request events.
Pull request event: Last merge commit on the GITHUB_REF branch
Push event: Commit pushed, unless deleting a branch (when it's the default branch)
https://help.github.com/en/actions/reference/events-that-trigger-workflows
|
|
||
| if (folder.length) { | ||
| return [ | ||
| `{BASE_URL}/${join(org, repo, 'tree', 'main', ...folder)}`, |
There was a problem hiding this comment.
This is because GitHub is starting to rename their default branches from master to main and I think this is supposed to become the site's default for new repos soon.
There was a problem hiding this comment.
Do you mind if we address this separately? There are a few places where master is hardcoded and I think this needs some additional thoughts
There was a problem hiding this comment.
Without this the test won't pass because the github/codeql-action repo uses main now.
There was a problem hiding this comment.
😄 Ahh I see, thanks for the hint.
|
This should be good now. |
Checklist:
This builds on the existing support and adds links for local actions in the current repo, as well as actions that have sub actions which are located in a sub folder for the given action.
I had to hard code
masterfor one of the urls because we don't have branch/commit information for it. What we could do is adjust the regex to capture the@v1value and use that. That should be a tag or commit which both work in the url and would be more accurate.Local
https://github.com/github/codeql-action/blob/a0d60d5d9e3126a00bed89fd89cc89b7e52a56f2/.github/workflows/codeql.yml#L24-L28
Sub folders
https://github.com/twbs/bootstrap/blob/5faf41eb4837491fa8193910c5816efadb4dbc5a/.github/workflows/codeql.yml#L22-L25