[ci-visibility] Fix GitHub Metadata Extraction #1771
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1771 +/- ##
=======================================
Coverage 98.21% 98.21%
=======================================
Files 931 931
Lines 44870 44872 +2
=======================================
+ Hits 44069 44071 +2
Misses 801 801
Continue to review full report at Codecov.
|
| @@ -213,16 +213,19 @@ def extract_github_actions(env) | |||
| ref = env['GITHUB_REF'] if ref.nil? || ref.empty? | |||
| branch, tag = branch_or_tag(ref) | |||
|
|
|||
| pipeline_url = "#{env['GITHUB_SERVER_URL']}/#{env['GITHUB_REPOSITORY']}/actions/runs/#{env['GITHUB_RUN_ID']}" | |||
There was a problem hiding this comment.
i. Are we guaranteed that GITHUB_SERVER_URL will be present, or should we have a fallback value for it?
ii. Also somewhat related, one thing we don't do in this file at all, but perhaps would be worth considering, is to use Hash#fetch instead of Hash#[] when we're fetching information we consider to be required.
For instance, in this case:
[1] pry(main)> env = {}
=> {}
[2] pry(main)> env['GITHUB_SERVER_URL']
=> nil
[3] pry(main)> env.fetch('GITHUB_SERVER_URL')
KeyError: key not found: "GITHUB_SERVER_URL"
from (pry):3:in `fetch'of course, this very much depends on what we want out of this process. Perhaps not having this failures so far is intentional, because we may prefer to have an incorrect metadata value, rather than a failure (...?).
There was a problem hiding this comment.
i. It seems so: https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables. This has consistently worked for us in our tests.
ii. Yeah I'd say it's safer to fallback to nil rather than failing. We follow CI's documentation pages for the name of these variables, but there are cases in which it's difficult to tell, for example, is GITHUB_REF_NAME defined for both PR and merge triggers? Leaving a key empty is safe: we just don't show a piece of metadata in our UI
There was a problem hiding this comment.
Note that in some cases, like this one, the key would not be empty, but actually incorrect, e.g. /actions/runs/ if all of the GITHUB_... env vars were missing.
Should we test that they exist before using them for templating?
There was a problem hiding this comment.
we work under the assumption that some env vars are always there. For example, if GITHUB_REPOSITORY is missing, we can't associate the test run to a repository, so most of our UI won't work. Test spans will still be in the event platform, with missing or invalid metadata, but I think that's fine, since we don't have any valid value to fallback to.
| "ci.pipeline.url": "https://github.com/ghactions-repo/commit/ghactions-commit/checks", | ||
| "ci.pipeline.url": "https://github.com/ghactions-repo/actions/runs/ghactions-pipeline-id/attempts/ghactions-run-attempt", |
There was a problem hiding this comment.
Can you confirm this is expected?
E.g. the git commit only mentions the introduction of env['GITHUB_SERVER_URL'] but there's a different feature being changed here, which is changing the pipeline url.
There was a problem hiding this comment.
yeah sry about that. I tend to always squash my PRs so I pay more attention to the PR description than the commit messages. This change is reflected in the description but not in the commit message.
There was a problem hiding this comment.
Ah, right. I totally missed it.
Changes
GITHUB_SERVER_URLinstead of hardcoding github.com."https://github.com/#{env['GITHUB_REPOSITORY']}/commit/#{env['GITHUB_SHA']}/checks"to"#{env['GITHUB_SERVER_URL']}/#{env['GITHUB_REPOSITORY']}/actions/runs/#{env['GITHUB_RUN_ID']}", which is the correct URL for pipelines in GitHub.github.json.