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

fix(ng-dev/release): run yarn integrity check before performing a release #264

Closed
wants to merge 3 commits into from

Conversation

josephperrott
Copy link
Member

If the ng-dev command is run with an outdate version of the package, a dev may perform a release without
the latest changes to the tooling. This can lead to not creating up to date artifacts and general issues.
By checking if the installed dependencies match the expected dependencies before performing the action we
can instruct the user to update their installed node_modules.

An example where this change would have prevented issue:
Framework releases from the patch branch which is currently a different version of the package as defined in
package.json. Calling them version 1 and version 2, with version 2 being the latest version. A patch release
is performed by checking out the latest commit from the upstream next branch, running yarn install and then
running ng-dev release publish. This release process will occur using version 2 of the dev-infra package.
During the release process, the patch branch is checked out and yarn install is performed, causing version 1
to be installed. Upon completion of the patch release, the next release is attempted unknowingly triggered
using version 1 of the package. While generated artifacts are already ensured to built with the correct
dependency by the process, the tool itself is already running before this check can occur. Instead version 1
of the package is used to process the release, potentially resulting in an errant or unexpected process.

This situation will now be prevented by validating the expected version is being used for the action process,
in the situation above the user would be informed that yarn install was needed before performing the second
release.

@josephperrott josephperrott added action: merge The PR is ready for merge by the caretaker action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 13, 2021
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Awesome! I wanted to do something manually with comparing versions, but if this works, that's definitely better.

*/
export async function invokeYarnIntegryCheck(projectDir: string): Promise<void> {
try {
await spawn('yarn', ['check', '--integrity'], {cwd: projectDir, mode: 'silent'});
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need https://classic.yarnpkg.com/en/docs/cli/check/#toc-yarn-check-verify-tree?

seems like the integrity check just checks the package json against the lock file unless I misunderstand the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we need both, now that I read it closer.

Because we want to make sure that our package.json matches installed deps and we want to make sure that our package.json matches our yarn.lock.

Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense, yeah 👍

…e performing a release

If the ng-dev command is run with an outdate version of the package, a dev may perform a release without
the latest changes to the tooling.  This can lead to not creating up to date artifacts and general issues.
By checking if the installed dependencies match the expected dependencies before performing the action we
can instruct the user to update their installed node_modules.

An example where this change would have prevented issue:
Framework releases from the patch branch which is currently a different version of the package as defined in
package.json. Calling them version 1 and version 2, with version 2 being the latest version. A patch release
is performed by checking out the latest commit from the upstream next branch, running yarn install and then
running ng-dev release publish.  This release process will occur using version 2 of the dev-infra package.
During the release process, the patch branch is checked out and yarn install is performed, causing version 1
to be installed.  Upon completion of the patch release, the next release is attempted unknowingly triggered
using version 1 of the package.  While generated artifacts are already ensured to built with the correct
dependency by the process, the tool itself is already running before this check can occur. Instead version 1
of the package is used to process the release, potentially resulting in an errant or unexpected process.

This situation will now be prevented by validating the expected version is being used for the action process,
in the situation above the user would be informed that yarn install was needed before performing the second
release.
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM

ng-dev/release/publish/external-commands.ts Outdated Show resolved Hide resolved
@josephperrott
Copy link
Member Author

This PR was merged into the repository by commit f31797a.

@josephperrott josephperrott deleted the integrity-check branch October 13, 2021 20:29
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker action: review The PR is still awaiting reviews from at least one requested reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants