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

Workflows: test to check for label and skip backport changelog #61808

Merged
merged 3 commits into from
May 21, 2024

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented May 20, 2024

What?

Follow up to:

Skips the backport changelog check in CI if the "No Core sync required" label is applied to a PR.

Why?

Folks might want to add changes that do not require a Core backport, e.g., syncing from Core to Gutenberg.

How?

Add an if condition to the action.

Testing Instructions

Make a PHP change in a new branch.

The check should fail.

Now add the "No Core sync required" or "Backport from WordPress Core" label

The CI should refresh and the check will not fail.

@ramonjd ramonjd requested a review from desrosj as a code owner May 20, 2024 22:20
Copy link

github-actions bot commented May 20, 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.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: priethor <priethor@git.wordpress.org>

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

@ramonjd ramonjd added the [Type] Build Tooling Issues or PRs related to build tooling label May 20, 2024
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/class-wp-rest-global-styles-controller-gutenberg.php

@ramonjd ramonjd added No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core and removed No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core labels May 20, 2024
@ramonjd ramonjd force-pushed the try/skip-backwards-compat-changelog-with-label branch from 868f3b7 to daf6c24 Compare May 20, 2024 22:35
@ramonjd ramonjd added No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core and removed No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core labels May 20, 2024
@ramonjd ramonjd force-pushed the try/skip-backwards-compat-changelog-with-label branch from b087b4d to 519383d Compare May 20, 2024 22:41
@@ -31,6 +31,7 @@ jobs:
- name: 'Fetch relevant history from origin'
run: git fetch origin ${{ github.event.pull_request.base.ref }}
- name: Check CHANGELOG status
if: ${{ !contains(github.event.pull_request.labels.*.name, 'No Core sync required') }}
Copy link
Member Author

@ramonjd ramonjd May 20, 2024

Choose a reason for hiding this comment

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

The target label could easily be "Backport from WordPress Core", but there might be other scenarios where a backport isn't required, e.g., adding gutenberg text domains to i18n translations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have multiple target labels? It would be really handy to look at "Backport from WordPress Core" too!

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I'll test.

Copy link
Member Author

@ramonjd ramonjd May 21, 2024

Choose a reason for hiding this comment

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

Done in 2ac0b2b

@@ -1,6 +1,6 @@
<?php
/**
* REST API: Try: bundle WP_Theme_JSON class instead of inheriting per WordPress version class
* REST API: Bundle WP_Theme_JSON class instead of inheriting per WordPress version class
Copy link
Member Author

Choose a reason for hiding this comment

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

Might as well leave this in. 😄

Copy link

github-actions bot commented May 20, 2024

Flaky tests detected in f4f5f3c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9178873250
📝 Reported issues:

@ramonjd ramonjd changed the title Workflows: test to check for label and skip check Workflows: test to check for label and skip backport changelog test May 20, 2024
@ramonjd ramonjd changed the title Workflows: test to check for label and skip backport changelog test Workflows: test to check for label and skip backport changelog May 20, 2024
@ramonjd ramonjd added Backport from WordPress Core Pull request that needs to be backported to a Gutenberg release from WordPress Core No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core and removed No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core Backport from WordPress Core Pull request that needs to be backported to a Gutenberg release from WordPress Core labels May 20, 2024
@ellatrix
Copy link
Member

An explicit label sounds good, yes 👍

@@ -31,6 +31,7 @@ jobs:
- name: 'Fetch relevant history from origin'
run: git fetch origin ${{ github.event.pull_request.base.ref }}
- name: Check CHANGELOG status
if: ${{ !contains(github.event.pull_request.labels.*.name, 'No Core sync required') && !contains(github.event.pull_request.labels.*.name, 'Backport from WordPress Core') }}
Copy link
Member

Choose a reason for hiding this comment

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

Are there other cases where no core sync is required, or is it just backports from core?

Copy link
Member

Choose a reason for hiding this comment

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

And a nit: could we title case the label? All other labels are, and most likely someone will, which will break the test

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively we could remove the label if you think there's no other types of changes except for backports from core. I think in other cases we should probably add it to the excluded paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are there other cases where no core sync is required, or is it just backports from core?

There are possibly several situations aside from backports:

  1. Experimental code wrapped in defined( 'IS_GUTENBERG_PLUGIN' )
  2. Experimental code in general :)
  3. Adding gutenberg text domains to i18n translations (needed in the plugin, but not in Core)
  4. Gutenberg hotfixes?
  5. I'm out of ideas.... 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

And a nit: could we title case the label? All other labels are, and most likely someone will, which will break the test

Good call. Will do, thanks!

@ellatrix ellatrix removed the No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core label May 21, 2024
@ellatrix ellatrix added the No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core label May 21, 2024
@ellatrix
Copy link
Member

Had to see test it for myself 😄

@ellatrix
Copy link
Member

Thanks for improving the script! ❤️

@priethor
Copy link
Contributor

An explicit label sounds good, yes 👍

Agreed, as it enables opting-out but the default is still to require backports. 💯

@ramonjd ramonjd merged commit f306978 into trunk May 21, 2024
62 checks passed
@ramonjd ramonjd deleted the try/skip-backwards-compat-changelog-with-label branch May 21, 2024 23:49
@github-actions github-actions bot added this to the Gutenberg 18.5 milestone May 21, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Jun 4, 2024
…ress#61808)

* Test to check for label and skip check

Test PHP change

Action

* Added extra label check

* Sentence case for the label: No Core Sync Required

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: priethor <priethor@git.wordpress.org>
patil-vipul pushed a commit to patil-vipul/gutenberg that referenced this pull request Jun 17, 2024
…ress#61808)

* Test to check for label and skip check

Test PHP change

Action

* Added extra label check

* Sentence case for the label: No Core Sync Required

Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: priethor <priethor@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants