feat: add variable `commit_date` #471

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

src/meta.ts

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

src/context.ts

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?

src/meta.ts

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

src/context.ts

+  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: trim21#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: trim21@4a4aa3e

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', () => {

__tests__/context.test.ts

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

trim21 force-pushed the commit-date branch from 67c7c03 to ac6abc8 Compare November 13, 2024 13:03

crazy-max requested changes Nov 13, 2024

View reviewed changes

src/meta.ts Outdated Show resolved Hide resolved

src/meta.ts Outdated Show resolved Hide resolved

src/meta.ts Outdated Show resolved Hide resolved

src/meta.ts Outdated Show resolved Hide resolved

trim21 force-pushed the commit-date branch 3 times, most recently from 25df83a to 02b92e6 Compare November 13, 2024 14:11

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

trim21 force-pushed the commit-date branch from e1cae70 to 6a7c003 Compare November 13, 2024 14:20

trim21 force-pushed the commit-date branch 2 times, most recently from 1434e0e to b63fcf7 Compare November 13, 2024 15:39

This comment was marked as outdated.

Sign in to view

trim21 force-pushed the commit-date branch 2 times, most recently from 9968536 to 1c7e57e Compare November 13, 2024 16:29

trim21 force-pushed the commit-date branch from 1c7e57e to b1cec0f Compare November 13, 2024 16:32

trim21 mentioned this pull request Nov 13, 2024

SOURCE_DATE_EPOCH is not respected #403

Open

trim21 force-pushed the commit-date branch 2 times, most recently from 12a07fa to 2ee1f2a Compare November 14, 2024 07:10

crazy-max reviewed Nov 14, 2024

View reviewed changes

src/context.ts Outdated Show resolved Hide resolved

src/context.ts Show resolved Hide resolved

trim21 force-pushed the commit-date branch from 2ee1f2a to e966ea1 Compare November 14, 2024 07:18

trim21 force-pushed the commit-date branch 2 times, most recently from 40465f6 to 69486da Compare November 14, 2024 07:27

crazy-max reviewed Nov 14, 2024

View reviewed changes

trim21 force-pushed the commit-date branch 2 times, most recently from 27580b6 to ce0e6b3 Compare November 14, 2024 07:50

crazy-max reviewed Nov 14, 2024

View reviewed changes

__tests__/context.test.ts Show resolved Hide resolved

__tests__/context.test.ts Show resolved Hide resolved

trim21 force-pushed the commit-date branch 4 times, most recently from 750c474 to b74a4a7 Compare November 16, 2024 17:00

feat: add variable commit_date

526d403

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

trim21 force-pushed the commit-date branch from b74a4a7 to 526d403 Compare November 16, 2024 17:04

crazy-max approved these changes Nov 18, 2024

View reviewed changes

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

This was referenced Nov 19, 2024

commit_date: code cleanup and readme updates #475

Merged

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add variable `commit_date` #471

feat: add variable `commit_date` #471

trim21 commented Nov 13, 2024

crazy-max commented Nov 13, 2024

trim21 commented Nov 13, 2024

crazy-max left a comment •

edited

Loading

trim21 commented Nov 13, 2024

This comment was marked as outdated.

trim21 commented Nov 13, 2024 •

edited

Loading

trim21 commented Nov 13, 2024 •

edited

Loading

trim21 commented Nov 14, 2024

trim21 commented Nov 14, 2024

crazy-max commented Nov 14, 2024 •

edited

Loading

trim21 commented Nov 14, 2024

crazy-max left a comment

crazy-max Nov 14, 2024

trim21 Nov 14, 2024

crazy-max Nov 14, 2024 •

edited

Loading

trim21 Nov 14, 2024 •

edited

Loading

crazy-max Nov 14, 2024

trim21 Nov 14, 2024

crazy-max left a comment

trim21 commented Nov 16, 2024

crazy-max left a comment

trim21 commented Nov 18, 2024

crazy-max commented Nov 18, 2024

trim21 commented Nov 18, 2024

feat: add variable commit_date #471

feat: add variable commit_date #471

Conversation

trim21 commented Nov 13, 2024

crazy-max commented Nov 13, 2024

trim21 commented Nov 13, 2024

crazy-max left a comment • edited Loading

Choose a reason for hiding this comment

trim21 commented Nov 13, 2024

This comment was marked as outdated.

trim21 commented Nov 13, 2024 • edited Loading

trim21 commented Nov 13, 2024 • edited Loading

trim21 commented Nov 14, 2024

trim21 commented Nov 14, 2024

crazy-max commented Nov 14, 2024 • edited Loading

trim21 commented Nov 14, 2024

crazy-max left a comment

Choose a reason for hiding this comment

crazy-max Nov 14, 2024

Choose a reason for hiding this comment

trim21 Nov 14, 2024

Choose a reason for hiding this comment

crazy-max Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

trim21 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

crazy-max Nov 14, 2024

Choose a reason for hiding this comment

trim21 Nov 14, 2024

Choose a reason for hiding this comment

crazy-max left a comment

Choose a reason for hiding this comment

trim21 commented Nov 16, 2024

crazy-max left a comment

Choose a reason for hiding this comment

trim21 commented Nov 18, 2024

crazy-max commented Nov 18, 2024

trim21 commented Nov 18, 2024

feat: add variable `commit_date` #471

feat: add variable `commit_date` #471

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