KAFKA-17540: Create script for updating a reference of latest cached trunk commit #17204

fonsdant · 2024-09-16T01:25:19Z

No description provided.

.github/workflows/build.yml

fonsdant · 2024-09-17T12:11:49Z

Hi, @mumrah! I have made a test to check if it works as expected. You can check it: https://github.com/fonsdant/github-actions.

To test, I have created two jobs: one for a successful build (and then tag update) and another for a failed build (no tag update). I have needed to give read and write permission to GitHub Action to perform the tag creation.

mumrah

Thanks @fonsdant! Left a few comments.

I also wonder if we should put this in the CI Complete workflow. Though unlikely, it is possible that the build succeeds, but writing to the cache fails in the Post Gradle Setup step. If this happened, we would have updated the tag prematurely.

If we add a "update-cached-tag" job in CI Complete, we can trigger it to only run when a CI workflow on trunk is successful (which would mean the cache was updated).

.github/workflows/build.yml

fonsdant · 2024-09-17T16:15:05Z

Thanks @fonsdant! Left a few comments.

I also wonder if we should put this in the CI Complete workflow. Though unlikely, it is possible that the build succeeds, but writing to the cache fails in the Post Gradle Setup step. If this happened, we would have updated the tag prematurely.

If we add a "update-cached-tag" job in CI Complete, we can trigger it to only run when a CI workflow on trunk is successful (which would mean the cache was updated).

Moved!

mumrah

One small thing remaining.

Also, can you see if signed tags are possible here? That might be a nice thing to include.

mumrah · 2024-09-17T22:51:32Z

.github/workflows/ci-complete.yml

+
+  update-cached-tag:
+    # Skip this workflow if the CI run was skipped or cancelled
+    if: (github.event.workflow_run.conclusion == 'success' || github.event.workflow_run.conclusion == 'failure')


In this case, we only want to run if the workflow was successful.

Oh, thanks! I will fix it!

fonsdant · 2024-09-18T15:33:10Z

One small thing remaining.

Also, can you see if signed tags are possible here? That might be a nice thing to include.

Yes, they are possible but require a GPG key. Is the purpose of the sign to be the same as the commits? If so, maybe we could add a Signed-off-by trailer at the end of the description. But I think it will require an automation, it is not out-of-the-box.

mumrah · 2024-09-18T16:17:58Z

Is the purpose of the sign to be the same as the commits?

No, the purpose is just to have a signature on the tag itself. When Github merges PRs, it will sign the resulting commit with its own gpg key. I was hoping that doing git tag -s in the action would do the same for tags. It should be possible to test it on your fork.

mumrah · 2024-09-19T01:55:57Z

I think tag signing with git may not work as expected here. It may be possible to use the API to create a signed tag. https://docs.github.com/en/rest/git/tags?apiVersion=2022-11-28

fonsdant · 2024-09-19T11:51:16Z

I think tag signing with git may not work as expected here.

I have made a test. Only to confirm: is it not work as expected because of this? Git could not determine GitHub bot's email.

Maybe we could use gh CLI to create the tag. GitHub CLI is preinstalled on all GitHub-hosted runners

mumrah · 2024-09-19T12:59:22Z

Yes, using gh should work. The docs link I posted has an example of creating a tag with gh api

gh api \
  --method POST \
  -H "Accept: application/vnd.github+json" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  /repos/OWNER/REPO/git/tags \
   -f "tag=v0.0.1" -f "message=initial version" -f "object=c3d0be41ecbe669545ee3e94d31ed9a4bc91ee3c" -f "type=commit" -f "tagger[name]=Monalisa Octocat" -f "tagger[email]=octocat@github.com" -f "tagger[date]=2011-06-17T14:53:35-07:00"

We're using this tool to create custom statuses on PRs here https://github.com/apache/kafka/blob/trunk/.github/actions/gh-api-update-status/action.yml#L55-L59

fonsdant · 2024-09-20T02:42:55Z

Oh! I have noted it fails when tag ref already exists. I am working on fix it!

mumrah · 2024-09-20T16:38:07Z

@fonsdant first off, thanks for all the work on this. I was pondering this yesterday and had an idea. Instead of managing a tag, I think we can use a separate branch, something like trunk-cached. This branch just contains the commits from trunk that are included in the latest GitHub cache.

In this case, the CI becomes simply

git checkout trunk-cached
git merge SHA
git push origin trunk-cached

This will improve the developer experience somewhat since it doesn't require explicitly fetching tags. Just a simple git fetch origin will pull it down.

Can you give this a try?

fonsdant · 2024-09-20T22:08:15Z

I am happy to help! :)

Sure! I will give it a try. It seems simpler for me too!

fonsdant · 2024-09-21T01:36:48Z

Hi, @mumrah! I have made some modifications. I have used switch as it seems more semantic. Instead of merge, I have chosen to use reset --hard to prevent any conflicts that merge could raise. How about it?

mumrah · 2024-09-21T02:13:13Z

.github/workflows/ci-complete.yml

+    steps:
+      - name: Update trunk-cached branch with trunk
+        run: |
+          git switch trunk-cached || git switch -c trunk-cached


Do we need to fetch trunk-cached first? Or does the checkout step fetch everything?

As we are doing a reset here, it is not needed to do fetch. Even if we do fetch, the fetched trunk-cached branch will be reset to github.sha as well as the unfetched trunk-cached branch. So the fetched trunk-cached does not change the result.

In a way, the github.sha is already our "fetched branch" or "reference", which the trunk-cached branch should point to. And push -f guarantees the update will be performed without fast-forward or similar issues.

I have done two tests for this. You can check them here and here.

This is the git log result:

$ git log --oneline e8da285 (HEAD -> trunk, origin/trunk-cached, origin/trunk) update ble.txt d9c105e minimal impl 425d23e replace sha with origin trunk branch ...

Thanks for the explanation. Looks like we're always doing git switch -c to create a local branch, so maybe we don't need the first git switch ?

You are right! I will remove the git switch. Thanks!!

mumrah · 2024-09-26T00:22:13Z

The new cut options are working 👍

$ git update-cache                 
264131cdaaef3f4696942f26534b3f61f3a2a162

mumrah · 2024-09-26T00:27:13Z

Ok, now we need to take the SHA captured by cut and use update-ref to create a local branch.

I'd like to see this output:

$ git update-cache
Local branch 'trunk-cached' updated to 264131cdaaef3f4696942f26534b3f61f3a2a162

BTW, does the gh cache command work for you as a non-committer?

fonsdant · 2024-09-26T00:29:12Z

Also, I wonder if we should do a git fetch origin as part of the command. Otherwise, we might pull in a SHA that doesn't exist locally

I think it will not work well for all cases. In my local repo, origin points to fonsdant/kafka (my fork) and upstream to apache/kafka. Maybe, we keep it more agnostic and print a message guiding the user to fetch. WDYT?

BTW, does the gh cache command work for you as a non-committer?

Yep! :)

$ git update-cache
264131cdaaef3f4696942f26534b3f61f3a2a162

mumrah · 2024-09-26T14:54:13Z

Maybe, we keep it more agnostic and print a message guiding the user to fetch. WDYT?

Good point on different remote names. Let's still attempt to update the ref in this script. If it can't be done, print a warning and recommend to the user to update their remote.

> git update-cache
Cannot update 'trunk-cached' because SHA 264131cdaaef3f4696942f26534b3f61f3a2a162 does not exist locally. Please update your remote and try again.
> echo $? 
1
> git fetch origin-or-whatever
> git update-cache
Local branch 'trunk-cached' updated to 264131cdaaef3f4696942f26534b3f61f3a2a162

committer-tools/update-cache.sh

mumrah · 2024-09-28T14:42:11Z

committer-tools/update-cache.sh

+  git switch trunk-cached &> /dev/null || git switch -c trunk-cached &> /dev/null
+  if git merge "$sha" &> /dev/null; then


Since this script is managing the trunk-cached ref exclusively, lets just use git update-ref.

git update-ref -m 'some message' trunk-cached $sha

if the SHA doesn't exist, it will fail with a non-zero exit code

It seems it is not working as expected... I am trying to update to 235cafa, but my trunk-cached still points to 1f04436de1. See:

$ git update-ref -m 'test' trunk-cached 235cafa805 $ echo $? 0 $ git log ... 1f04436de1 (trunk-cached) Merge commit '1854d4b8a11461b53b59fa109b95f2a4f5003997' into trunk-cached ...

But using git branch -f has worked well:

$ git branch -f trunk-cached 235cafa805 $ git log ... 235cafa805 (trunk-cached) KAFKA-6197: Update Streams API and Javadoc references in documentation (#17215) ...

committer-tools/update-cache.sh

Signed-off-by: Joao Pedro Fonseca Dantas <fonsdant@gmail.com>

fonsdant · 2024-09-29T00:06:28Z

The script has failed with rev-parse (could not to find commit locally), so I have replaced it with show.

mumrah

Thanks for the updates @fonsdant! I tried it out and it worked as expected. Nice!

Just two small things then I'm happy to merge this.

@chia7712 did you have any additional comments?

mumrah · 2024-09-29T03:30:37Z

docs/ops.html

@@ -1249,7 +1249,7 @@ <h4 class="anchor-heading"><a id="prodconfig" class="anchor-link"></a><a href="#

  <h3 class="anchor-heading"><a id="java" class="anchor-link"></a><a href="#java">6.6 Java Version</a></h3>

-  Java 8, Java 11, Java 17, and Java 21 are supported.
+  Java 8, Java 11, Java 17, Java 21, and Java 23 are supported.


Was this change intentional?

Oh! It should not have been sent actually. Thanks! I will remove it.

mumrah · 2024-09-29T03:33:51Z

committer-tools/update-cache.sh

+
+sha="$(cut -d '-' -f 5 <<< "$key")"
+
+git fetch &> /dev/null


The behavior of git fetch with no args depends on the local config, so it might not be the same for everyone.

Since we are expecting developers to have done the appropriate git fetch or git pull prior to running this command, I think we can omit the git fetch completely from this script.

Make sense, I will proceed this way!

…on-trunk-for-ci-cache

mumrah

LGTM!

I'm going to go ahead and merge this after incorporating to two minor nitpicks from the last review. Thanks for sticking with us through this PR @fonsdant 😄. I think we ended up trying three totally different approaches before settling on the local script.

chia7712 · 2024-10-01T14:29:14Z

@fonsdant thanks for this great contribution!

fonsdant · 2024-10-01T15:48:46Z

Thank you very much, @mumrah, @chia7712! I have learned a lot along this PR! 😁

add cache tag step in validate job

590f748

fonsdant marked this pull request as draft September 16, 2024 01:25

fonsdant changed the title ~~KAFKA-17540: Create floating tag on trunk for CI cache~~ [WIP] KAFKA-17540: Create floating tag on trunk for CI cache Sep 16, 2024

mumrah reviewed Sep 16, 2024

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

mumrah added the build Gradle build or GitHub Actions label Sep 16, 2024

restrict cached tag update to trunk

84e9aba

fonsdant changed the title ~~[WIP] KAFKA-17540: Create floating tag on trunk for CI cache~~ KAFKA-17540: Create floating tag on trunk for CI cache Sep 17, 2024

fonsdant marked this pull request as ready for review September 17, 2024 12:11

mumrah reviewed Sep 17, 2024

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

.github/workflows/build.yml Outdated Show resolved Hide resolved

.github/workflows/build.yml Outdated Show resolved Hide resolved

fonsdant added 4 commits September 17, 2024 12:11

only fetch cached tag

2b40661

annotate tag with a default auto generated message

ac04af8

remove redundant condition from step name

8cbea90

move update cache tag step to ci complete workflow

626000d

mumrah reviewed Sep 17, 2024

View reviewed changes

only update cached tag if success

e020e80

replace git with gh cli

249a544

fonsdant requested a review from mumrah September 20, 2024 02:25

replace gh cli for tag with git for branch

a953e41

mumrah reviewed Sep 21, 2024

View reviewed changes

mumrah reviewed Sep 26, 2024

View reviewed changes

committer-tools/update-cache.sh Outdated Show resolved Hide resolved

fonsdant added 6 commits September 27, 2024 21:35

replace alias creation with suggestion

2dbfa55

update local branch to cache

a452d50

add failure message

f71a09c

replace merge with rebase

651b677

replace rebase with merge

a67a25b

remove ff only option

c9e4600

fonsdant requested a review from mumrah September 28, 2024 01:52

mumrah reviewed Sep 28, 2024

View reviewed changes

fonsdant added 5 commits September 28, 2024 12:10

replace merge origin with all option

f7b14e0

remove fetch all option

acb7b0e

replace git rev parse with git show

9e38188

Signed-off-by: Joao Pedro Fonseca Dantas <fonsdant@gmail.com>

remove switch, add branch f, and truncate sha

9dccc41

Signed-off-by: Joao Pedro Fonseca Dantas <fonsdant@gmail.com>

truncate sha failure message

be79fe9

Signed-off-by: Joao Pedro Fonseca Dantas <fonsdant@gmail.com>

fonsdant requested a review from mumrah September 29, 2024 00:07

mumrah reviewed Sep 29, 2024

View reviewed changes

mumrah requested a review from chia7712 September 29, 2024 03:35

mumrah added 3 commits October 1, 2024 09:56

two small changes

02859b2

Merge remote-tracking branch 'origin/trunk' into create-floating-tag-…

fa9b4d8

…on-trunk-for-ci-cache

add relative date

47c4d73

mumrah approved these changes Oct 1, 2024

View reviewed changes

mumrah changed the title ~~KAFKA-17540: Create floating tag on trunk for CI cache~~ KAFKA-17540: Create script for updating a reference of latest cached trunk commit Oct 1, 2024

mumrah merged commit 84bcdc9 into apache:trunk Oct 1, 2024
7 checks passed

		git switch trunk-cached &> /dev/null \|\| git switch -c trunk-cached &> /dev/null
		if git merge "$sha" &> /dev/null; then

KAFKA-17540: Create script for updating a reference of latest cached trunk commit #17204

KAFKA-17540: Create script for updating a reference of latest cached trunk commit #17204

Conversation

fonsdant commented Sep 16, 2024

fonsdant commented Sep 17, 2024 • edited Loading

mumrah left a comment

Choose a reason for hiding this comment

fonsdant commented Sep 17, 2024

mumrah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fonsdant commented Sep 18, 2024

mumrah commented Sep 18, 2024

mumrah commented Sep 19, 2024

fonsdant commented Sep 19, 2024 • edited Loading

mumrah commented Sep 19, 2024

fonsdant commented Sep 20, 2024

mumrah commented Sep 20, 2024

fonsdant commented Sep 20, 2024

fonsdant commented Sep 21, 2024

Choose a reason for hiding this comment

fonsdant Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumrah commented Sep 26, 2024

mumrah commented Sep 26, 2024

fonsdant commented Sep 26, 2024

mumrah commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fonsdant commented Sep 29, 2024

mumrah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumrah left a comment

Choose a reason for hiding this comment

chia7712 commented Oct 1, 2024

fonsdant commented Oct 1, 2024

fonsdant commented Sep 17, 2024 •

edited

Loading

fonsdant commented Sep 19, 2024 •

edited

Loading

fonsdant Sep 21, 2024 •

edited

Loading