Skip to content

Update precommit:emoji script after GitHub's sunset of SVN#6107

Closed
desrosj wants to merge 12 commits intoWordPress:trunkfrom
desrosj:update/precommit-emoji
Closed

Update precommit:emoji script after GitHub's sunset of SVN#6107
desrosj wants to merge 12 commits intoWordPress:trunkfrom
desrosj:update/precommit-emoji

Conversation

@desrosj
Copy link
Member

@desrosj desrosj commented Feb 13, 2024

All build script related changes form #5804 have been copied over.

Trac ticket: https://core.trac.wordpress.org/ticket/60520


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@desrosj desrosj self-assigned this Feb 13, 2024
@github-actions
Copy link

github-actions bot commented Feb 13, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props desrosj, kraftbj, peterwilsoncc, swissspidy.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions
Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@kraftbj
Copy link

kraftbj commented Feb 16, 2024

Tested. Only thing I ran into was permission needing updating on the shell script. Submitted a PR to add that via desrosj#153

Update file permissions for the shell script
@desrosj
Copy link
Member Author

desrosj commented Feb 16, 2024

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).

@kraftbj
Copy link

kraftbj commented Feb 16, 2024

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 desrosj#154 -- can you give that a spin?

@WordPress WordPress deleted a comment from ak10512106 Feb 22, 2024
Add script directly to Gruntfile.js
@desrosj
Copy link
Member Author

desrosj commented Feb 26, 2024

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.

@kraftbj
Copy link

kraftbj commented Feb 26, 2024

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.

@desrosj desrosj force-pushed the update/precommit-emoji branch from 6f2e0ea to e57fef2 Compare February 26, 2024 18:10
@desrosj
Copy link
Member Author

desrosj commented Feb 26, 2024

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
Copy link
Contributor

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.

This prevents upstream changes from breaking our test scripts.
# Tests the WordPress Core build process on multiple operating systems.
test-core-build-process:
name: Core running from ${{ matrix.directory }}
uses: WordPress/wordpress-develop/.github/workflows/callable-test-core-build-process.yml@trunk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To undo before commit

@desrosj
Copy link
Member Author

desrosj commented Mar 4, 2024

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 since this adds an additional spot to update when updating the library, it would be nice to automate it a bit.

@desrosj
Copy link
Member Author

desrosj commented Mar 4, 2024

@desrosj desrosj closed this Mar 4, 2024
@desrosj desrosj deleted the update/precommit-emoji branch March 4, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants