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

ci: add prerelease #2553

Closed
wants to merge 2 commits into from
Closed

ci: add prerelease #2553

wants to merge 2 commits into from

Conversation

btaillon-coveo
Copy link
Contributor

@btaillon-coveo btaillon-coveo commented Nov 16, 2022

I'm merging this in master since in theory it shouldn't affect it. Changes from #2548 were copied here.

New prerelease process

To start a major prerelease (e.g., 1.2.32.0.0-pre.0):

  1. Make sure that HEAD refers to a version bump on master.
  2. Create a new branch prefixed with prerelease/ (e.g., prerelease/headless_atomic_v2)
  3. Run npm run bump:version:major-prerelease -- @coveo/package1 @coveo/package2 @coveo/package3.
  4. Push the new commit and its tags to the branch.

Updating a prerelease branch:

  1. Wait for master to reference a version bump.
  2. Pull changes from master into the prerelease branch.

Adding new changes to the prerelease branch:

  1. Open pull requests towards the release branch
  2. Squash as usual.

Officially releasing (e.g., 2.0.0-pre.152.0.0):

  1. Create a pull request from the prerelease branch to “master”.
    • Associate with a JIRA which QA will use to validate.
    • Update the changelogs manually
    • Update all dependents to use the prerelease version (include the -pre. suffix)
  2. Squash after a version bump on master.

Changes in this PR

  1. Added a bump:version:major-prerelease script to the root package.json
    • This script will create a major prerelease version bump on the current branch, and will give it all the appropriate tags, but won't push it.
    • The commit will contain the same message that lerna gives version bumps, including [skip-ci].
    • Changelogs aren't affected.
  2. Non-bump commits on a prerelease/** branch now trigger the same CI as master, except:
    • Only packages matching /pre\.[0-9]+$/ are upgraded.
      • If a dependent is out of scope (doesn't match the above version pattern), its dependencies won't be upgraded.
    • Changelogs aren't updated.
  3. Bump commits on a prerelease/** branch now trigger the same CI as master, except:
    • Only packages matching /pre\.[0-9]+$/ are published to NPM.
    • The alpha tag on NPM isn't updated.
    • The package rollout stops at the dev environment.
  4. Some scripts were converted from CommonJS (.js) to ESM (.cjs)
  5. Various calls to execSync were replaced with a utility execute function
    • execute, in addition to returning its output, copies stdout & stderr to the console to make debugging easier. I caught some issues early thanks to this.

Other details

  • Packages are going to be published to the CDN based on their major/minor/patch version, the prerelease version will be ignored. Therefor, 2.0.0 will already be available on the dev CDN, and will be overwritten every time we update the prerelease. Out of scope packages will also update their CDN, but in theory the files should be the same anyway.

https://coveord.atlassian.net/browse/KIT-2114

Comment on lines +11 to +13
/**
* @typedef {import('../packages.mjs').PackageDefinition} PackageDefinition
*/
Copy link
Contributor Author

@btaillon-coveo btaillon-coveo Nov 16, 2022

Choose a reason for hiding this comment

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

This is the JSDoc equivalent to import type { PackageDefinition } from '../packages';.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice touch

)
.join('\n');
const message = `${lernaMessageSection}\n\n${updatedPackagesMessageSection}`;
await execute('git', ['commit', '--no-verify', '-m', message]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this, and multi-line commits work.

@btaillon-coveo btaillon-coveo marked this pull request as ready for review November 16, 2022 21:45
@btaillon-coveo btaillon-coveo requested review from a team as code owners November 16, 2022 21:45
@github-actions
Copy link

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 216 216 0
search 312.1 312.1 0
insight 258.1 258.1 0
product-listing 229.5 229.5 0
product-recommendation 213.7 213.7 0
recommendation 213.8 213.8 0

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

Nothing seems particularly wrong, tho there's too many things for me to judge/give an approve given my limited knowledge of your CI/CD

Comment on lines +62 to +82
export function updatePackageVersion(
packageName,
newVersion,
depdendenciesPackageDirs
) {
depdendenciesPackageDirs.forEach((packageDir) => {
const fullPath = resolve('.', 'packages', packageDir, 'package.json');
const originalContentAsText = readFileSync(fullPath).toString();
let newContent = originalContentAsText.replace(
new RegExp(`("${packageName.replace('/', '\\/')}"\\s*:\\s*")([^"]*)`),
'$1' + newVersion
);
if (packageName === JSON.parse(originalContentAsText).name) {
newContent = newContent.replace(
/("version"\s*:\s*\")([^"]*)/,
'$1' + newVersion
);
}
writeFileSync(fullPath, newContent);
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not parse, edit then, write? It'll be more maintainable than a RegExp nah?

Copy link
Contributor Author

@btaillon-coveo btaillon-coveo Nov 17, 2022

Choose a reason for hiding this comment

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

Ah I should've clarified this part. Unfortunately, parsing, editing then reformatting gives a pretty ugly JSON, and I wanted to maintain the current structure of the JSON.

Copy link
Collaborator

Choose a reason for hiding this comment

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

* @param {readonly string[]} [args]
* @returns {Promise<string>}
*/
export function execute(command, args = []) {
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 add a package to do this type of thing? I feel like it's a bit out of our scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to see the command & output of every command in the console while also being able to use the output, which by default isn't the case with spawn and exec (setting stdio: 'inherit' makes them return nothing). I could've tried a bunch of third-party exec libraries to do it, but it doesn't seem to be worth the time exploring them, and this is a fairly simple utility function that mostly just wraps the native spawn function.

@ThibodeauJF
Copy link
Contributor

ThibodeauJF commented Nov 17, 2022

Nice work!

It would be nice to update the readme for upcoming pre-releases.

Some nitpick: doing the refactor to modules in the same PR as adding the pre-release specific code & the migration from Lerna... it will make it hard to discern changes when looking back. These kind of things were a bit annoying when fixing something in the dep. process in the past.

I'd wait for Olivier's feedback too before merging

@btaillon-coveo
Copy link
Contributor Author

Nice work!

It would be nice to update the readme for upcoming pre-releases.

Some nitpick: doing the refactor to modules in the same PR as adding the pre-release specific code & the migration from Lerna... it will make it hard to discern changes when looking back. These kind of things were a bit annoying when fixing something in the dep. process in the past.

I'd wait for Olivier's feedback too before merging

Agreed, I ideally would've liked to do it separately, but unfortunately I wasn't able to use the semantic monorepo tools with commonJS.

@ThibodeauJF
Copy link
Contributor

What if it was

  1. PR1: switch to monorepo tools for our current release process + mjs conversion
  2. Test that the main release process still works properly & iterate
  3. PR2: add prerelease
  4. Test, iterate...

@btaillon-coveo
Copy link
Contributor Author

What if it was

  1. PR1: switch to monorepo tools for our current release process + mjs conversion
  2. Test that the main release process still works properly & iterate
  3. PR2: add prerelease
  4. Test, iterate...

We can do that, but in this case PR1 may be as big as this PR (possibly bigger since we'd need to add changelogs and conventional graduating). It would be more consistent across both releases though. Is this what you prefer?

@louis-bompart
Copy link
Collaborator

louis-bompart commented Nov 17, 2022

PR1: cjs -> mjs
PR2: monorepo tools
PR3: prerelease
Etc?

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.

3 participants