feat: add variable commit_date
#471
Conversation
|
#470 has been merged, can you rebase please? |
|
I find a problem, we add |
There was a problem hiding this comment.
Missing js artifacts build with docker buildx bake pre-checkin, see: https://github.com/docker/metadata-action/blob/master/.github/CONTRIBUTING.md#submitting-a-pull-request
Missing DCO as well https://github.com/docker/metadata-action/pull/471/checks?check_run_id=32924130175
|
it will use local event data file for push event, and fallback to github api for PR. |
This comment was marked as outdated.
This comment was marked as outdated.
|
I may need to update GitHub mocking logic for better code style |
|
done |
|
review applied |
|
forget to run eslint 😅 |
|
Can you add to: Line 17 in 32323e5 |
|
done |
| const commit = await toolkit.github.octokit.request('GET /repos/{owner}/{repo}/commits/{commit_sha}', { | ||
| commit_sha: sha, | ||
| owner: GitHub.context.repo.owner, | ||
| repo: GitHub.context.repo.repo | ||
| }); |
There was a problem hiding this comment.
I don't think this would work for pull requests from a fork
There was a problem hiding this comment.
There was a problem hiding this comment.
Looking at https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#get-a-commit--parameters I think we can just pass the full ref like:
| const commit = await toolkit.github.octokit.request('GET /repos/{owner}/{repo}/commits/{commit_sha}', { | |
| commit_sha: sha, | |
| owner: GitHub.context.repo.owner, | |
| repo: GitHub.context.repo.repo | |
| }); | |
| const commit = await toolkit.github.octokit.request('GET /repos/{owner}/{repo}/commits/{GitHub.context.ref}', { | |
| owner: GitHub.context.repo.owner, | |
| repo: GitHub.context.repo.repo | |
| }); |
In case of a pull request it would be smth like 'GET /repos/{owner}/{repo}/commits/pull/471/merge' for your pr.
There was a problem hiding this comment.
no, a commit in a fork will also always exists in any forks. we do not need to handle this
for example: https://github.com/trim21/metadata-action/commit/4a4aa3e4cba597bb90d7342e9a2da7891cb822eb
https://api.github.com/repos/trim21/metadata-action/commits/4a4aa3e4cba597bb90d7342e9a2da7891cb822eb
There was a problem hiding this comment.
see this: trim21#2
I meant for a pull request from a fork but seems GitHub handles that: https://github.com/docker/metadata-action/actions/runs/11832905758/job/32970605385?pr=471#step:3:18
There was a problem hiding this comment.
in anyway, if sha is correct, we will get a commit. even if we send request to head repo not base repo
There was a problem hiding this comment.
Overall LGTM thanks!
Can we have more test cases in meta.test.ts to cover other kind of events:
- push:
metadata-action/__tests__/meta.test.ts
Line 130 in 44d81d6
- pr:
metadata-action/__tests__/meta.test.ts
Line 2268 in 44d81d6
- pr-head-sha:
metadata-action/__tests__/meta.test.ts
Line 2627 in 44d81d6
- schedule:
metadata-action/__tests__/meta.test.ts
Line 2994 in 44d81d6
- release:
metadata-action/__tests__/meta.test.ts
Line 3250 in 44d81d6
Signed-off-by: Trim21 <trim21.me@gmail.com>
|
done |
|
is there a release cycle? |
Yes probably this week |
|
I forget to add this to toc |
No description provided.