feat: add variable `commit_date` by trim21 · Pull Request #471 · docker/metadata-action

trim21 · 2024-11-13T13:01:45Z

No description provided.

crazy-max · 2024-11-13T13:03:02Z

#470 has been merged, can you rebase please?

trim21 · 2024-11-13T13:04:47Z

I find a problem, we add Git.commitDate as async function but everything in Meta is synchronously...

crazy-max

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

trim21 · 2024-11-13T14:22:45Z

it will use local event data file for push event, and fallback to github api for PR.

trim21 · 2024-11-13T16:19:27Z

I may need to update GitHub mocking logic for better code style

trim21 · 2024-11-13T16:30:02Z

done

trim21 · 2024-11-14T07:10:47Z

review applied

trim21 · 2024-11-14T07:18:50Z

forget to run eslint 😅

crazy-max · 2024-11-14T07:23:14Z

Can you add

core.info(`commitDate: ${context.commitDate}`);

to:

metadata-action/src/main.ts

Line 17 in 32323e5

await core.group(`Context info`, async () => {

trim21 · 2024-11-14T07:24:48Z

done

crazy-max

I just merged #472 to check the behavior for git context, can you rebase?

crazy-max · 2024-11-14T07:44:59Z

+  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
+  });


I don't think this would work for pull requests from a fork

see this: https://github.com/trim21/metadata-action/pull/2

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:

Suggested change

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.

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

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

in anyway, if sha is correct, we will get a commit. even if we send request to head repo not base repo

crazy-max

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

describe('push', () => {
pr:

metadata-action/__tests__/meta.test.ts

Line 2268 in 44d81d6

describe('pr', () => {
pr-head-sha:

metadata-action/__tests__/meta.test.ts

Line 2627 in 44d81d6

describe('pr-head-sha', () => {
schedule:

metadata-action/__tests__/meta.test.ts

Line 2994 in 44d81d6

describe('schedule', () => {
release:

metadata-action/__tests__/meta.test.ts

Line 3250 in 44d81d6

describe('release', () => {

Signed-off-by: Trim21 <[email protected]>

trim21 · 2024-11-16T17:10:40Z

done

crazy-max

LGTM thanks!

trim21 · 2024-11-18T13:55:45Z

is there a release cycle?

crazy-max · 2024-11-18T14:13:01Z

is there a release cycle?

Yes probably this week

trim21 · 2024-11-18T22:27:47Z

I forget to add this to toc

@crazy-max

crazy-max requested changes Nov 13, 2024

View reviewed changes

Comment thread src/meta.ts Outdated

Comment thread src/meta.ts Outdated

Comment thread src/meta.ts Outdated

Comment thread src/meta.ts Outdated

trim21 marked this pull request as ready for review November 13, 2024 14:13

trim21 changed the title ~~add variable commit_date~~ feat: add variable commit_date Nov 13, 2024

This comment was marked as outdated.

Sign in to view

trim21 mentioned this pull request Nov 13, 2024

SOURCE_DATE_EPOCH is not respected #403

Open

crazy-max reviewed Nov 14, 2024

View reviewed changes

Comment thread src/context.ts Outdated

Comment thread src/context.ts

crazy-max reviewed Nov 14, 2024

View reviewed changes

Comment thread __tests__/context.test.ts

Comment thread __tests__/context.test.ts

feat: add variable commit_date

526d403

Signed-off-by: Trim21 <[email protected]>

crazy-max approved these changes Nov 18, 2024

View reviewed changes

crazy-max merged commit c85c22a into docker:master Nov 18, 2024

This was referenced Nov 19, 2024

commit_date: code cleanup and readme updates #475

Merged

Cannot read properties of null (reading 'date') #476

Closed

This was referenced Dec 6, 2024

Proposol: add commit date variable #292

Closed

Take leverage of variables into labels #488

Closed

polarathene mentioned this pull request Jan 27, 2026

Better document implicit defaults of labels/annotations #592

Open

Conversation

trim21 commented Nov 13, 2024

Uh oh!

crazy-max commented Nov 13, 2024

Uh oh!

trim21 commented Nov 13, 2024

Uh oh!

crazy-max left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trim21 commented Nov 13, 2024

Uh oh!

This comment was marked as outdated.

trim21 commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trim21 commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trim21 commented Nov 14, 2024

Uh oh!

Uh oh!

Uh oh!

trim21 commented Nov 14, 2024

Uh oh!

crazy-max commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trim21 commented Nov 14, 2024

Uh oh!

crazy-max left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crazy-max Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

trim21 Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

crazy-max Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trim21 Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazy-max Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

trim21 Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

crazy-max left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

trim21 commented Nov 16, 2024

Uh oh!

crazy-max left a comment

Choose a reason for hiding this comment

Uh oh!

trim21 commented Nov 18, 2024

Uh oh!

crazy-max commented Nov 18, 2024

Uh oh!

trim21 commented Nov 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

crazy-max left a comment •

edited

Loading

trim21 commented Nov 13, 2024 •

edited

Loading

trim21 commented Nov 13, 2024 •

edited

Loading

crazy-max commented Nov 14, 2024 •

edited

Loading

crazy-max Nov 14, 2024 •

edited

Loading

trim21 Nov 14, 2024 •

edited

Loading