#60520 closed task (blessed) (fixed)
Update `precommit:emoji` following GitHub's deprecation of SVN support.
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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.
2 years ago
#1
- Keywords has-patch added
#2
@
2 years 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.
2 years 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
2 years 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
ghas a requirement for the script. - I would prefer that the necessary script be included in
Grunt.jsinstead of a separate directory for shell scripts (where this will be the only one).
2 years 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?
2 years 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.
2 years 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.
2 years 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:
2 years 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.
2 years ago
#11
As the script currently reads the files from the
mainbranch, 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
@
2 years 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