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

Optimize workflows; Add Dependabot and CodeQL #973

Open
1 of 6 tasks
thelovekesh opened this issue Feb 6, 2024 · 12 comments
Open
1 of 6 tasks

Optimize workflows; Add Dependabot and CodeQL #973

thelovekesh opened this issue Feb 6, 2024 · 12 comments
Assignees
Labels
Infrastructure Issues for the overall performance plugin infrastructure

Comments

@thelovekesh
Copy link
Member

thelovekesh commented Feb 6, 2024

Currently, there are a few areas where workflows can be optimized to reduce CI times, maintenance, and security in the runners. Also, consider adding tools like @dependabot that can keep the dependency up-to-date unless some dependency requires it, for example: chalk since it's pure ESM now and the plugin CLI is CJS.

Tasks

  • Optimize workflows for lower CI times and reduce files for maintenance(i.e. lint workflows can remain in one file).
  • Remove dependency from third-party workflows for tasks that can be handled natively.
  • Update permissions in the workflows to read-only and manually provide any other permission at the job level.
  • Update wp-env based PHPUnit setup with MySQL + SVN which is easy to set up for any version and has very less or no compatibility maintenance.
  • Add @dependabot config to update composer, npm, and github-actions at least once a month.
  • Add CodeQL for JS code analysis for any security vulnerabilities.
@westonruter
Copy link
Member

I presume you'll be opening separate PRs for each tasks for review?

@thelovekesh
Copy link
Member Author

Yes, that would be like:

  • First three tasks in one PR since they are optimization changes.
  • Fourth task in standalone PR since it will refactor PHPUnit setup.
  • Last two can be included in single PR since their config files are plain and simple.

@swissspidy
Copy link
Member

Some quick thoughts on thelovekesh#2 after taking another look:

  • .github/release.yml is not needed. We don't currently use GitHub's auto-generated release notes feature.
  • The whole determine-file-counts logic seems a bit overkill and adds complexity. Do we really need that?
  • What's the cache busting for. Never had to use something like that in a project before. Why is it needed here?

@thelovekesh
Copy link
Member Author

Thanks, @swissspidy for taking a look. Here are my few observations:

.github/release.yml is not needed. We don't currently use GitHub's auto-generated release notes feature.

Since we are going to add @dependabot && if we ever require to use the GitHub notes release feature, this config can come in handy. We can skip adding it, but I don't see any problem in keeping it too.

The whole determine-file-counts logic seems a bit overkill and adds complexity. Do we really need that?

Currently, there are standalone workflow files for JS and PHP that do the same tasks like linting, testing, etc. Let's say tomorrow we need to add linting for MD files, will we be adding a new workflow file for that? I think it is better to consolidate all linting tasks in a single file, testing tasks in a single file, and so on. The above logic can help determine change paths on the job level which is a well-used thing in multiple projects.

What's the cache busting for. Never had to use something like that in a project before. Why is it needed here?

I think GitHub doesn't delete a cache if it's being used frequently. The above workflow busts that cache weekly.

@swissspidy
Copy link
Member

if we ever require to use the GitHub notes release feature, this config can come in handy. We can skip adding it, but I don't see any problem in keeping it too.

It's a misleading to have a config file for a feature that's not being used. It's like dead code. We can just add it when we need it.

I think it is better to consolidate all linting tasks in a single file, testing tasks in a single file, and so on.

Not disagreeing with that part. Consolidating makes sense. And I understand why you added the logic for determining changed files etc. But do we really need it? What's the drawback of not adding it? Will the workflows really be that slower?

@thelovekesh
Copy link
Member Author

What's the drawback of not adding it? Will the workflows really be that slower?

I think it's about running relevant workflows on the changes made in a PR. If the changes are only JS, do we need to run other workflows for like 1-2 minutes(for linting) and maybe 3-4 minutes(for testing)? while the logic for determining changed files only takes 2-3 seconds.

@thelovekesh
Copy link
Member Author

I have found some issues in wp-env based unit tests setup where it seems like Imagick is not working as expected. See #1013 (comment)

/cc @westonruter @swissspidy @mukeshpanchal27 @joemcgill for visibility

@swissspidy
Copy link
Member

Sounds like we can make the tests more robust, good :)

@thelovekesh
Copy link
Member Author

Created #1085 to discuss task 4 of this issue more widely.
cc @swissspidy for visibility.

@swissspidy swissspidy changed the title Optimize workflows; Add @dependabot and CodeQL Optimize workflows; Add Dependabot and CodeQL Mar 26, 2024
@adamsilverstein
Copy link
Member

adamsilverstein commented May 1, 2024

I have found some issues in wp-env based unit tests setup where it seems like Imagick is not working as expected. See #1013 (comment)

can we use GD instead of Imagick to address this?

@swissspidy
Copy link
Member

I think there are some dedicated unit tests for both GD and Imagick, so ideally we have a setup that has both

@westonruter
Copy link
Member

An initial Dependabot config was added by @mukeshpanchal27 in #1355

@swissspidy swissspidy added the Infrastructure Issues for the overall performance plugin infrastructure label Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure
Projects
None yet
Development

No branches or pull requests

4 participants