Details

Reviewers

tomek
• jacek
• abosh
varun
ashoat

Commits

rCOMM003e533f4a6d: [CI] Introduce Harbormaster tag removal script generator

Summary

This script

Uses the Phabricator differential.query API (https://phab.comm.dev/conduit/method/differential.query/) to determine which revisions of diffs are no longer relevant (because the diffs have been closed or abandoned)
Writes a new shell script with a series of git commands to remove the git tags associated with those no longer relevant revisions.

The way this script will be used on the CI will be something like:

cd scripts && node generate-phab-removal-script.js
chmod +x tag_removal_script.sh
./tag_removal_script.sh

Test Plan

Tested locally (with the steps above) and removed a few thousand extraneous tags.

What happens if the script attempts to remove a tag that has already been removed from the repo?

That's fine it's a noop that logs the following:

remote: warning: deleting a non-existent ref

The way the script is written now, we will definitely be attempting to remove tags that have already been removed since we aren't checking whether the tag already exists on remote. This is fine because the remove tag command is idempotent, but I'll address it anyways in a subsequent diff.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Jul 31 2022, 12:04 PM

Herald added a subscriber: • adrian. · View Herald TranscriptJul 31 2022, 12:04 PM

remove extraneous fn argument

Harbormaster completed remote builds in B10993: Diff 15127.Jul 31 2022, 12:18 PM

Harbormaster completed remote builds in B10994: Diff 15128.Jul 31 2022, 12:23 PM

atul requested review of this revision.Jul 31 2022, 12:23 PM

atul edited the summary of this revision. (Show Details)Jul 31 2022, 12:28 PM

atul added a child revision: D4690: [CI] Add removal of irrelevant Harbormaster git tags to GH Actions.Jul 31 2022, 1:03 PM

Why the nested scripts?

Looking at this more, it seems like:

Easier to call git from shell
Easier to call gaxios from JS

Overall this looks good to me, but would love for somebody with more bash experience to take a look. Separately, it might make review a bit easier if @atul could share an example of a generated shell script.

atul mentioned this in D4690: [CI] Add removal of irrelevant Harbormaster git tags to GH Actions.Aug 1 2022, 7:01 AM

Separately, it might make review a bit easier if @atul could share an example of a generated shell script.

Agree with this.

The logic here makes sense and the script looks good (I stepped through the output of a sample differential.query API method call), but on the Phabricator page you linked it says:

This method is frozen and will eventually be deprecated. New code should use "differential.revision.search" instead.

Is there a reason we can't use differential.revision.search? I stepped through the output of a sample method call for it and it looks like it has the same info? That is, information regarding "Abandoned/Closed" diffs:

Although not so sure about if the ID is the same (the "diffs" field you used on line 28). Haven't really looked into it too much, but wanted to know if there was a reason we can't use the more stable method

This revision now requires changes to proceed.Aug 1 2022, 7:56 AM

Separately, it might make review a bit easier if @atul could share an example of a generated shell script.

git push --delete origin tag phabricator/base/15140
git push --delete origin tag phabricator/diff/15140
git push --delete origin tag phabricator/base/15138
git push --delete origin tag phabricator/diff/15138
git push --delete origin tag phabricator/base/15131
git push --delete origin tag phabricator/diff/15131
git push --delete origin tag phabricator/base/15139
git push --delete origin tag phabricator/diff/15139
git push --delete origin tag phabricator/base/15126
git push --delete origin tag phabricator/diff/15126
git push --delete origin tag phabricator/base/15113
git push --delete origin tag phabricator/diff/15113
git push --delete origin tag phabricator/base/15093
git push --delete origin tag phabricator/diff/15093
git push --delete origin tag phabricator/base/15112
git push --delete origin tag phabricator/diff/15112
...

Is there a reason we can't use differential.revision.search?

The default output of differential.revision.search doesn't include the diffs field which has the revision IDs associated with each diff. I'm sure we could use whatever sequence of "non-frozen" endpoints to get the information we need, but differential.query gives it to us all in one go.

Haven't really looked into it too much, but wanted to know if there was a reason we can't use the more stable method

If I'd realized that the differential.query endpoint was "frozen," I'd have looked into using other endpoints.. but given the logic is written using differential.query and this is a pretty low importance script, I think it's fine to leave as is. Phabricator is "no longer actively maintained" as of June 1, 2021 so I doubt they'll ever remove this endpoint.

It's also not quite "deprecated" but "will eventually be deprecated," so I think it's fine.

I'm sure we could use whatever sequence of "non-frozen" endpoints to get the information we need, but differential.query gives it to us all in one go...given the logic is written using differential.query and this is a pretty low importance script, I think it's fine to leave as is.

That makes sense to me! Thanks for clarifying.

This revision is now accepted and ready to land.Aug 1 2022, 8:27 AM

atul added a reviewer: varun. atul added 1 blocking reviewer(s): tomek.Aug 1 2022, 8:30 AM

This revision now requires review to proceed.Aug 1 2022, 8:30 AM

varun accepted this revision.Aug 1 2022, 8:58 AM

rebase before landing

atul removed 1 blocking reviewer(s): tomek.Aug 1 2022, 10:36 AM

This revision is now accepted and ready to land.Aug 1 2022, 10:36 AM

Harbormaster failed remote builds in B11010: Diff 15146!Aug 1 2022, 10:40 AM

rebase

Harbormaster completed remote builds in B11013: Diff 15149.Aug 1 2022, 10:54 AM

Harbormaster completed remote builds in B11010: Diff 15146.