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

Utilize post install Script for Composer #32657

Merged
merged 8 commits into from
May 3, 2022

Conversation

ObliviousHarmony
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

This pull request adds composer install to the postinstall action for our plugins. During this work I also noticed that prepare scripts are executing on install, and as a consequence, every package is being built. This was previously necessary, but with our build orchestration tooling, we can rely on that build packages as needed. This should remove any ambiguity around having to run the composer-install executor, as well as speed up updates since they won't build packages anymore.

Closes #32652.

How to test the changes in this Pull Request:

  1. Remove all node_modules and vendor folders in your cloned directory
  2. Run pnpm install, it should not build any packages. It will still build the core plugin, however, due to a separate install script in composer.json.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file by running pnpm nx affected --target=changelog?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@ObliviousHarmony ObliviousHarmony added the focus: monorepo infrastructure Issues and PRs related to monorepo tooling. label Apr 15, 2022
@ObliviousHarmony ObliviousHarmony requested review from a team and psealock and removed request for a team April 15, 2022 22:55
@github-actions github-actions bot added package: @woocommerce/e2e-environment Issues related to @woocommerce/e2e-environment package. package: @woocommerce/api Issues related to @woocommerce/api package. package: @woocommerce/e2e-core-tests Issues related to @woocommerce/e2e-core-tests package. package: @woocommerce/e2e-utils Issues related to @woocommerce/e2e-utils package. package: @woocommerce/admin-e2e-tests issues related to @woocommerce/admin-e2e-tests plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Apr 15, 2022
Copy link
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

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

This is testing well and changes make sense. This part though isn't actually correct:

Run pnpm install, it should not build any packages. It will still build the core plugin

WC Admin client isn't built, so only a portion of WooCommerce is useable. Unless this is on purpose and we like the extra command or we're making the decision to handle this later on when the Admin client is migrated, we could change this line to prepare.

"prepack": "pnpm install && pnpm run lint && pnpm run test && cross-env WC_ADMIN_PHASE=core pnpm run build",

The rest of the changes are great 👍🏽

@ObliviousHarmony ObliviousHarmony force-pushed the refactor/standardize-postinstall branch 2 times, most recently from d302ff3 to f9c1a81 Compare April 28, 2022 20:49
@ObliviousHarmony
Copy link
Contributor Author

@psealock The work that was blocking this PR has been resolved in #32804. This PR is ready now. The test failures in the E2E related tests are unrelated and going to be fixed in #32820.

@ObliviousHarmony
Copy link
Contributor Author

ObliviousHarmony commented Apr 28, 2022

Actually, with the caching @roykho added in #32725, I think I need to add an environment variable that can be set to bypass the installation. I'll check and see, since technically composer install should see that the vendor folder is there and do nothing? Do we need to check for the presence of the cache before installing?

@psealock
Copy link
Contributor

Do we need to check for the presence of the cache before installing?

Composer will avoid installing if vendor/ is present and the lock file is up to date.

I think I need to add an environment variable that can be set to bypass the installation

When would you like to bypass installation? When CI actions use a cache? In that case, probably doesn't matter

@ObliviousHarmony ObliviousHarmony force-pushed the refactor/standardize-postinstall branch 4 times, most recently from 2d51130 to 04438a1 Compare April 29, 2022 19:17
@github-actions github-actions bot added the package: @woocommerce/api-core-tests Issues related to @woocommerce/api-core-tests package. label Apr 29, 2022
Thanks to our build orchestration tooling, there is
no reason for us to build all of the packages on
`install`. In cases where `prepare` was used to
build before packaging, this changes it to
`prepack` so it will run only under the correct
circumstances.
This commit adds a `postinstall` script for
the WooCommerce and WooCommerce Beta
Tester plugins. This makes it easier to get
started since `pnpm install` will ensure all
dependencies in every project are ready.
Like Core's `composer.json`, we need to override
the platform in order to get a consistent version
for dependencies out of Composer.
@ObliviousHarmony ObliviousHarmony force-pushed the refactor/standardize-postinstall branch from cd4fba0 to 9a22c62 Compare May 2, 2022 19:56
Copy link
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

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

I think it would be good to get this merged as is and address remaining questions, if any, later on.

@ObliviousHarmony ObliviousHarmony merged commit 790d085 into trunk May 3, 2022
@ObliviousHarmony ObliviousHarmony deleted the refactor/standardize-postinstall branch May 3, 2022 17:21
@github-actions github-actions bot added this to the 6.6.0 milestone May 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2022

Hi @ObliviousHarmony, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: monorepo infrastructure Issues and PRs related to monorepo tooling. package: @woocommerce/admin-e2e-tests issues related to @woocommerce/admin-e2e-tests package: @woocommerce/api Issues related to @woocommerce/api package. package: @woocommerce/api-core-tests Issues related to @woocommerce/api-core-tests package. package: @woocommerce/e2e-core-tests Issues related to @woocommerce/e2e-core-tests package. package: @woocommerce/e2e-environment Issues related to @woocommerce/e2e-environment package. package: @woocommerce/e2e-utils Issues related to @woocommerce/e2e-utils package. plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Utilize postinstall Script for Composer Dependencies
2 participants