#60520 closed task (blessed) (fixed)
Update `precommit:emoji` following GitHub's deprecation of SVN support.
Reported by: | peterwilsoncc | Owned by: | desrosj |
---|---|---|---|
Milestone: | 6.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | has-patch needs-testing |
Focuses: | Cc: |
Description
In a move few people noticed, GitHub have deprecated their support of SVN.
As the emoji:precommit
script makes use of SVN to obtain a list of files in the twemoji repository, this has broken the update of formatting.php
by the precommit script.
At present, this leaves committers to manually update the formatting file via alternative methods. Let's explore something that will work for all.
A workaround was created for PR#5804 but discussing this with @desrosj we decided to action the build step in a follow up ticket in the hope of avoiding introducing the GitHub CLI as a requirement.
Follow up to #57600
Change History (12)
This ticket was mentioned in PR #6107 on WordPress/wordpress-develop by @desrosj.
10 months ago
#1
- Keywords has-patch added
#2
@
10 months ago
- Keywords needs-testing added
- Milestone changed from Awaiting Review to 6.5
- Type changed from defect (bug) to task (blessed)
After [57626], I've created a PR with only the build related changes to work on this.
10 months ago
#3
Tested. Only thing I ran into was permission needing updating on the shell script. Submitted a PR to add that via https://github.com/desrosj/wordpress-develop/pull/153
10 months ago
#4
Thanks @kraftbj! I've merged that.
To bring a brief summary here, here are a few things I don't love about the current implementation (though not strongly opposed to):
- I don't love adding
gh
as a requirement for the script. - I would prefer that the necessary script be included in
Grunt.js
instead of a separate directory for shell scripts (where this will be the only one).
10 months ago
#5
I don't love adding gh
either, but given that we _do_ use GitHub as a partner service extensively already, it seems okay for now to get this out the door.
I tried to make it all part of the Grunt JS file in https://github.com/desrosj/wordpress-develop/pull/154 -- can you give that a spin?
10 months ago
#7
Thanks all for keeping this going! I think we're at a good spot. There's just one outstanding question about what seems like an unintentional deleted line.
10 months ago
#8
I concur. If the deletion was intentional (I'm not sure why it would be), it should be noted in the commit to explain the connection. But, with that line restored, LGTM.
10 months ago
#9
It was me that removed that line, wasn't it? Just realized that looking through this again. 🙃
Since I can't recall why I included that, or see a reason how it's related, I've gone and removed it.
I also added a new step to the Actions workflow that tests our build process to run the precommit:emoji
script to confirm all expected changes are included. GitHub is not syncing pushes right now for some reason. But once I confirm it correctly flags missing changes, I'll get this merged.
@peterwilsoncc commented on PR #6107:
10 months ago
#10
I also added a new step to the Actions workflow that tests our build process to run the precommit:emoji script to confirm all expected changes are included. GitHub is not syncing pushes right now for some reason. But once I confirm it correctly flags missing changes, I'll get this merged.
As the script currently reads the files from the main
branch, this may need to use something version specific so it doesn't trigger build failures due to modified files as the folder is updated upstream.
10 months ago
#11
As the script currently reads the files from the
main
branch, this may need to use something version specific so it doesn't trigger build failures due to modified files as the folder is updated upstream.
I've tested using a tag reference instead of branch, and it seems to work alright. Talked through this a bit in Slack with @peterwilsoncc and it seems better to tackle this separately. Since it adds an additional spot to update when updating the library, it would be nice to automate it a bit.
#12
@
10 months ago
- Owner set to desrosj
- Resolution set to fixed
- Status changed from new to closed
In 57758:
All build script related changes form #5804 have been copied over.
Trac ticket: https://core.trac.wordpress.org/ticket/60520