Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update google fonts on release instead of on a schedule #461

Merged
merged 6 commits into from
Oct 31, 2023

Conversation

vcanales
Copy link
Member

@vcanales vcanales commented Oct 24, 2023

Currently, the data from the Google Fonts API is being updated every Tuesday, but pulling the new data makes sense closer to a release since, before that, current plugin installations won't consume it.

Here, I propose moving the automated workflow of the update to the Release Workflow so that this may be done when needed.

Testing:

  • On a fork, checkout this branch as trunk
  • If you want to test the actual file update, modify your copy of assets/google-fonts/fallback-fonts-list.json.
  • Comment out this line, otherwise, the step won't run.
  • Run the workflow (Go to Actions > Deploy to WorkPress.org, choose any version increase)

The "Update Google Fonts JSON file" and "Commit Changes" steps should pass without errors.

Note that in your fork, the full workflow will report a failure, since you'll be missing the secrets necessary to actually deploy the plugin; this is expected.

@vcanales vcanales added the enhancement New feature or request label Oct 24, 2023
@vcanales vcanales marked this pull request as ready for review October 24, 2023 18:53
@vcanales vcanales force-pushed the update-google-fonts-on-release branch from bd7d8b5 to 32643de Compare October 31, 2023 18:09
Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Tested in a fork and the "Update Google Fonts JSON file" and "Commit Changes" steps passed without errors. Code looks good to me!

I think this is ready to bring in - I have one question but it's not a blocker.

.github/workflows/deploy-to-dotorg.yml Outdated Show resolved Hide resolved
@vcanales vcanales merged commit d6fb04f into trunk Oct 31, 2023
1 check passed
creativecoder added a commit to creativecoder/create-block-theme that referenced this pull request Nov 1, 2023
@creativecoder
Copy link
Contributor

I reviewed this post-merge, mostly to learn more about how create-block-theme releases work.

Thanks for the specific testing instructions, everything worked as expected on my fork (I did need to add a GOOGLE_FONTS_API_KEY env secret first, since I didn't have that).

One risk this approach seems to create is that it removes a manual review before committing the Google fonts configuration, since that's now tied to an automated release process. For example, if the fonts API request returned successfully, but had erroneous data, like an empty array of fonts, I think that would be committed automatically to the plugin as is was released. Is that right?

It's an unlikely scenario, so not a huge risk. But a nice improvement might be some basic checking of the fonts API response in the update script to make sure it looks to as expected, and throwing an error if any problems are detected.

@mikachan mikachan deleted the update-google-fonts-on-release branch November 3, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants