Skip to content

Conversation

@Tjitse-E
Copy link
Contributor

@Tjitse-E Tjitse-E commented Sep 7, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Currently the PHPCS baseline tests are failing when there are no files to check. See https://github.com/mage-os/mageos-magento2/actions/runs/5794755408/job/15940229468?pr=32.

What is the new behavior?

  • Stop before composer install step when phpcs has no files to check
  • Only run phpcs for files in the app dir
  • Simplify action, by utilising the build in function of tj-actions/changed-files to check which files were modified.
  • Trigger error when phpcs baseline was not created.
  • Run phpcs on all filetypes that are specified in the Magento ruleset

Does this PR introduce a breaking change?

  • Yes
  • No

Additional information

  • Example run with adding an additional error in app/code/Magento/Catalog/view/frontend/templates/product/breadcrumbs.phtml, while there are existing errors in breadcrumbs.phtml (that are now in the baseline, so shoudn't be reported).
  • Example run when there are no relevant phpcs files changed.

Related PR

mage-os/mageos-magento2#32

@Tjitse-E Tjitse-E requested a review from a team as a code owner September 11, 2023 17:45
@Tjitse-E
Copy link
Contributor Author

@Vinai i've resolved the issue that occured last friday (the baseline test didn't work on forked repo's). We now have a successfull run: https://github.com/mage-os/mageos-magento2/actions/runs/6149981258/job/16686980485?pr=34.

@Vinai
Copy link
Contributor

Vinai commented Sep 12, 2023

Thank you, I will review!
Can you join us today at 3pm CET for the weekly tech meeting by chance?
It's in the voice channel on Discord.

Copy link
Contributor

@Vinai Vinai left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

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.

2 participants