feat(sync): Addition of the sync command to the deploy pipeline #2997

sreenara · 2023-08-10T04:50:39Z

COMPLETES #SPARK-449508

This pull request addresses

In order to allow for packages to be published with all of the correct versions of their dependencies, a new @webex/package-tools command must be introduced and injected into the deploy pipeline that effectively collects and stores all package versions for a specific branch/release on all packages prior to publishing.

by making the following changes

This PR creates the sync command which takes in 2 arguments

tag - Reference to the branch that the packages need to be synched with
packages - List of packages to sync. This is an optional argument and if not provided, the command will recursively sync all packages in the repo

deploy.yml has also been modified to run the sync command before incrementing the versions of the packages using the increment command.

Change Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Tooling change
Internal code refactor

The following scenarios where tested

Running
yarn package-tools sync --tag next
yarn package-tools increment --packages @webex/plugin-meetings

@webex/plugin-meetings/package.json

Before:

   "devDependencies": {
    "@babel/core": "^7.17.10",
    "@types/jsdom": "^21",
    "@webex/babel-config-legacy": "workspace:^",
    "@webex/eslint-config-legacy": "workspace:^",
    "@webex/jest-config-legacy": "workspace:^",
    "@webex/legacy-tools": "workspace:^",
    "@webex/plugin-meetings": "workspace:^",
    "@webex/plugin-rooms": "workspace:^",
    "@webex/test-helper-chai": "workspace:^",
    "@webex/test-helper-mocha": "workspace:^",
    "@webex/test-helper-mock-webex": "workspace:^",
    "@webex/test-helper-retry": "workspace:^",
    "@webex/test-helper-test-users": "workspace:^",
    "chai": "^4.3.4",
    "chai-as-promised": "^7.1.1",
    "eslint": "^8.24.0",
    "jsdom": "19.0.0",
    "jsdom-global": "3.0.2",
    "prettier": "^2.7.1",
    "sinon": "^9.2.4",
    "typescript": "^4.7.4"
  },
  "dependencies": {
    "@webex/common": "workspace:^",
    "@webex/internal-media-core": "0.0.7-beta",
    "@webex/internal-plugin-device": "workspace:^",
    "@webex/internal-plugin-metrics": "workspace:^",
    "@webex/internal-plugin-support": "workspace:^",
    "@webex/internal-plugin-user": "workspace:^",
    "@webex/plugin-people": "workspace:^",
    "@webex/ts-sdp": "1.0.1",
    "@webex/webex-core": "workspace:^",
    "bowser": "^2.11.0",
    "btoa": "^1.2.1",
    "dotenv": "^4.0.0",
    "global": "^4.4.0",
    "ip-anonymize": "^0.1.0",
    "javascript-state-machine": "^3.1.0",
    "lodash": "^4.17.21",
    "sdp-transform": "^2.12.0",
    "uuid": "^3.3.2",
    "webrtc-adapter": "^7.7.0"
  }

After:

   "devDependencies": {
    "@babel/core": "^7.17.10",
    "@types/jsdom": "^21",
    "@webex/babel-config-legacy": "^0.0.0",
    "@webex/eslint-config-legacy": "^0.0.0",
    "@webex/jest-config-legacy": "^0.0.0",
    "@webex/legacy-tools": "^0.0.0",
    "@webex/plugin-meetings": "^2.59.0-next.1",
    "@webex/plugin-rooms": "^2.59.0-next.0",
    "@webex/test-helper-chai": "^2.59.0-next.0",
    "@webex/test-helper-mocha": "^2.59.0-next.0",
    "@webex/test-helper-mock-webex": "^2.59.0-next.0",
    "@webex/test-helper-retry": "^2.59.0-next.0",
    "@webex/test-helper-test-users": "^2.59.0-next.0",
    "chai": "^4.3.4",
    "chai-as-promised": "^7.1.1",
    "eslint": "^8.24.0",
    "jsdom": "19.0.0",
    "jsdom-global": "3.0.2",
    "prettier": "^2.7.1",
    "sinon": "^9.2.4",
    "typescript": "^4.7.4"
  },
  "dependencies": {
    "@webex/common": "^2.59.0-next.0",
    "@webex/internal-media-core": "0.0.7-beta",
    "@webex/internal-plugin-device": "^2.59.0-next.0",
    "@webex/internal-plugin-metrics": "^2.59.0-next.0",
    "@webex/internal-plugin-support": "^2.59.0-next.0",
    "@webex/internal-plugin-user": "^2.59.0-next.0",
    "@webex/plugin-people": "^2.59.0-next.0",
    "@webex/ts-sdp": "1.0.1",
    "@webex/webex-core": "^2.59.0-next.0",
    "bowser": "^2.11.0",
    "btoa": "^1.2.1",
    "dotenv": "^4.0.0",
    "global": "^4.4.0",
    "ip-anonymize": "^0.1.0",
    "javascript-state-machine": "^3.1.0",
    "lodash": "^4.17.21",
    "sdp-transform": "^2.12.0",
    "uuid": "^3.3.2",
    "webrtc-adapter": "^7.7.0"
  },

I certified that

I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly

Make sure to have followed the contributing guidelines before submitting.

mkesavan13

A couple of petty changes in the review. Majorly, I think the flow should be,

Sync npmjs version to all of the package.json (have a map of these like now)
Perform increment command and update the map above with changed package versions
Now, do syncDependency on changed packages alone
Perform npm publish on changed packages alone

Right now, it doesn't do Step 1 I believe.

@InteractiveTimmy - Please correct me if am wrong.

packages/tools/package/src/commands/sync/sync.constants.ts

mkesavan13 · 2023-08-10T12:42:49Z

packages/tools/package/src/commands/sync/sync.ts

+  /**
+   * Configuration Object for this increment Command configuration.
+   */
+  config: CONSTANTS.CONFIG,
+
+  /**
+   * Handles incrementing packages based on the provided Options from a
+   * Commands class instance.
+   *
+   * @param options - Options provided from the CLI interface.
+   * @returns - Promise that resolves once the process is complete.
+   */


Please update the comments here

Updated this. Will push this in the next commit

packages/tools/package/src/models/package/package.ts

sreenara · 2023-08-10T15:34:33Z

Sync npmjs version to all of the package.json (have a map of these like now)

@mkesavan13 can you elaborate on this one? Not able to understand this point. What would this map look like?

I agree that if the increment is done after sync, we might have a situation such as this:
Existing versions:
@webex/package-a: 2.59.0
@webex/package-b: 2.59.0 (dependent on a)

After increment
@webex/package-a - 2.59.0.next-1
@webex/package-b (dependent on a) - 2.59.0.next-1

However, in the package.json of @webex/package-b - dependency of @webex/package-a would still be 2.59.0 while publishing? @InteractiveTimmy am I correct?

InteractiveTimmy

Some changes needed around the pipeline changes and the syncing of the dependencies, devDependencies, etc not being needed.

If you find that when just adding the version key:value to all packages, the packed sub-package still doesn't have the correct versions, please reach out to me.

InteractiveTimmy · 2023-08-10T15:52:39Z

.github/workflows/deploy.yml

@@ -171,7 +171,7 @@ jobs:
          key: dist-${{ env.rid }}

      - name: Synchronize Packages
-        run: yarn
+        run: yarn package-tools sync --tag ${GITHUB_REF##*/}


A caveat for yarn workspaces in GitHub Actions is that we will need to run yarn as well as yarn package-tools sync --tag ${GITHUB_REF##*/}. Since each job on GitHub Actions starts with a fresh instance, it needs to align all local packages after uncaching dependencies. Example:

run: | yarn yarn package-tools sync --tag ${GITHUB_REF##*/}

yarn must be ran once first on any job that needs to use yarn in a workspaces environment [after uncaching]

InteractiveTimmy · 2023-08-10T16:49:45Z

packages/tools/package/src/models/package/package.ts

+   *
+   * @returns - Promse that resolves to this Package instance.
+   */
+  public syncDependency(deps: Record<string, string>, devDeps: boolean = false): Promise<this> {


syncDependencies may not be needed. when we run yarn npm publish on a sub-package using yarn workspaces it looks for the version key:value in the package.json for all dependent packages automatically and writes them for us. We should only need to put the correct and current version key:value into each sub-packages package.json, and not touch the dependencies list within each package.

I thought so too. But I was confused if semantic-release did this for us all this while or the yarn npm publish

Thanks for the clarification @InteractiveTimmy

@InteractiveTimmy @mkesavan13 I've removed syncDependency based on the discussion

mkesavan13 · 2023-08-11T07:29:39Z

@sreenara - To respond to your comment and Since @InteractiveTimmy mentioned that the dependent package versions will be changed by yarn npm publish automatically which makes sense because the workspaces are a feature of yarn and they won't let us do this by ourselves, writing down the following elaboration,

1. Sync npmjs version to all of the package.json

Right now, package.json doesn't have version numbers.
Therefore, the main aim of the sync command is to get the current version of all of the packages from the view() util method (which will get the version from npmjs.com cloud using the npm view command)
Add those version numbers of each packages to their respective package.json

2. Perform increment command

Once the package.json has the current version numbers added, we need to perform increment
This increment command should happen only on the changed packages (the yarn_recursive will have this list)
At the end of this exercise, the package.json of all the changed packages will have the version number incremented

3. syncDependency on changed packages alone

We DO NOT need this as it will be taken care by yarn itself

4. Perform npm publish on changed packages alone

Once again iterate through yarn_recursive list and publish the changed packages alone#2997

@InteractiveTimmy - Please correct me if something is wrong. Thank you.

sreenara · 2023-08-13T17:42:26Z

@sreenara - To respond to your comment and Since @InteractiveTimmy mentioned that the dependent package versions will be changed by yarn npm publish automatically which makes sense because the workspaces are a feature of yarn and they won't let us do this by ourselves, writing down the following elaboration,

1. Sync npmjs version to all of the package.json

Right now, package.json doesn't have version numbers.

Therefore, the main aim of the sync command is to get the current version of all of the packages from the view() util method (which will get the version from npmjs.com cloud using the npm view command)

Add those version numbers of each packages to their respective package.json

2. Perform increment command

Once the package.json has the current version numbers added, we need to perform increment

This increment command should happen only on the changed packages (the yarn_recursive will have this list)

At the end of this exercise, the package.json of all the changed packages will have the version number incremented

3. syncDependency on changed packages alone

We DO NOT need this as it will be taken care by yarn itself

4. Perform npm publish on changed packages alone

Once again iterate through yarn_recursive list and publish the changed packages alone#2997

@InteractiveTimmy - Please correct me if something is wrong. Thank you.

I've made changes in the code as per our discussion. Agreed, we don't need syncDependency any more.

sreenara · 2023-08-14T03:36:54Z

After:

   "devDependencies": {
    "@babel/core": "^7.17.10",
    "@types/jsdom": "^21",
    "@webex/babel-config-legacy": "^0.0.0",
    "@webex/eslint-config-legacy": "^0.0.0",
    "@webex/jest-config-legacy": "^0.0.0",
    "@webex/legacy-tools": "^0.0.0",

@InteractiveTimmy While doing a yarn pack on the sub-package @webex/plugin-meetings, the legacy packages are getting nulled to 0.0.0 because we don't have folders for them in the repo. How do we handle this?

mkesavan13

Looks good to me. Elegant implementation. Just one minor change.

Also just out of curiosity, aren't we implementing the publish step as well as part of this ticket? @sreenara @InteractiveTimmy

mkesavan13 · 2023-08-14T06:29:47Z

packages/tools/package/src/commands/sync/sync.ts

+
+    const tag = options.tag.split('/').pop();
+
+    console.log('sreenara ', Yarn);


Can this console log be removed?

sreenara · 2023-08-14T17:31:12Z

Also just out of curiosity, aren't we implementing the publish step as well as part of this ticket? @sreenara

@mkesavan13 We can do that as part of the freezing of the master branch and using next from then.

InteractiveTimmy · 2023-08-14T16:53:24Z

packages/tools/package/src/models/package/package.types.ts

+
+  /**
+   * Dependencies of the package
+   */
+  dependencies: Record<string, string>;
+
+  /**
+   * Developer dependencies of the package
+   */
+  devDependencies: Record<string, string>;


I don't believe these are needed, please remove. Let me know if I'm missing where they are used.

@InteractiveTimmy they aren't being used any more. Have removed them.

InteractiveTimmy · 2023-08-14T17:22:30Z

packages/tools/package/src/commands/sync/sync.ts

+        ? options.packages.includes(pack.name)
+        : true)))
+      .then((packs) => Promise.all(packs.map((pack) => pack.inspect())))
+      .then((packs) => Promise.all(packs.map((pack) => pack.syncVersion())))


One small change needed: syncVersion was designed to synchronize a branch/release tag to the latest tag version [keeps all feature branches in line with master/main/latest]. You should not need this call at all, since inspect() performs the remote-registry check on NPMJS and prepares the version stored in memory for the apply() command, which saves the memory-stored version to the disk.

Feel free to remove line 62 all together.

Some Notes

In the case that we are attempting to inspect a package for either the sync or increment commands:

If the NPMJS package exists, but the tag does not

We use the latest version with an amended new tag

If the NPMJS package does not exist

We throw an error and halt all additional pipeline steps for publishing/incrementing

We will be generating a publish workflow for new packages at a later time, this is outside of the scope of this pull request.

@InteractiveTimmy I've removed the line that does the syncVersion. You're right, that's not needed.
Any ideas about the issue with the legacy packages that don't have folders which get nulled to 0.0.0?

sreenara · 2023-08-16T04:16:30Z

After:
   "devDependencies": {
    "@babel/core": "^7.17.10",
    "@types/jsdom": "^21",
    "@webex/babel-config-legacy": "^0.0.0",
    "@webex/eslint-config-legacy": "^0.0.0",
    "@webex/jest-config-legacy": "^0.0.0",
    "@webex/legacy-tools": "^0.0.0",
@InteractiveTimmy While doing a yarn pack on the sub-package @webex/plugin-meetings, the legacy packages are getting nulled to 0.0.0 because we don't have folders for them in the repo. How do we handle this?

As discussed over Webex, these will get fixed during the first publish

InteractiveTimmy

LGTM!

…x#2997)

sreenara added 3 commits August 4, 2023 14:20

fix(test): test commit to try out increment and pipeline

ed87deb

feat(sync): add sync command and basic setup

6ce91c4

feat(sync): additional changes for sync

01ab630

sreenara added the validated If the pull request is validated for automation. label Aug 10, 2023

sreenara added 2 commits August 10, 2023 13:48

fix(nit): remove unnecessary changes and code

a32d930

fix(test): added tests for sync

7f4711a

sreenara marked this pull request as ready for review August 10, 2023 12:24

sreenara requested a review from a team as a code owner August 10, 2023 12:24

mkesavan13 requested changes Aug 10, 2023

View reviewed changes

arun3528 reviewed Aug 10, 2023

View reviewed changes

packages/tools/package/src/models/package/package.ts Outdated Show resolved Hide resolved

InteractiveTimmy requested changes Aug 10, 2023

View reviewed changes

feat(sync): removed syncDependency and incorporated comments

3f16582

fix(deploy): added yarn command to get packages first

915ba13

mkesavan13 approved these changes Aug 14, 2023

View reviewed changes

fix(sync): remove unnecessary console log

201ebd3

InteractiveTimmy requested changes Aug 14, 2023

View reviewed changes

feat(sync): removed unnecessary call to syncVersion

98ecb06

InteractiveTimmy approved these changes Aug 16, 2023

View reviewed changes

fix(actions): fix actions version

87d279c

sreenara merged commit a4c6b48 into webex:next Aug 16, 2023
10 of 11 checks passed

rajeshtezu pushed a commit to rajeshtezu/webex-js-sdk that referenced this pull request Aug 21, 2023

feat(sync): Addition of the sync command to the deploy pipeline (webe…

90eafc5

…x#2997)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sync): Addition of the sync command to the deploy pipeline #2997

feat(sync): Addition of the sync command to the deploy pipeline #2997

sreenara commented Aug 10, 2023 •

edited

mkesavan13 left a comment •

edited

mkesavan13 Aug 10, 2023

sreenara Aug 10, 2023 •

edited

sreenara commented Aug 10, 2023 •

edited

InteractiveTimmy left a comment

InteractiveTimmy Aug 10, 2023

sreenara Aug 13, 2023

InteractiveTimmy Aug 10, 2023

mkesavan13 Aug 11, 2023

sreenara Aug 13, 2023

mkesavan13 commented Aug 11, 2023

sreenara commented Aug 13, 2023

1. Sync npmjs version to all of the package.json

2. Perform increment command

3. syncDependency on changed packages alone

4. Perform npm publish on changed packages alone

sreenara commented Aug 14, 2023

mkesavan13 left a comment •

edited

mkesavan13 Aug 14, 2023

sreenara Aug 14, 2023

sreenara commented Aug 14, 2023

InteractiveTimmy Aug 14, 2023 •

edited

sreenara Aug 15, 2023

InteractiveTimmy Aug 14, 2023

sreenara Aug 15, 2023

sreenara commented Aug 16, 2023

InteractiveTimmy left a comment


		const tag = options.tag.split('/').pop();

		console.log('sreenara ', Yarn);

feat(sync): Addition of the sync command to the deploy pipeline #2997

feat(sync): Addition of the sync command to the deploy pipeline #2997

Conversation

sreenara commented Aug 10, 2023 • edited

COMPLETES #SPARK-449508

This pull request addresses

by making the following changes

Change Type

The following scenarios where tested

I certified that

mkesavan13 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sreenara Aug 10, 2023 • edited

Choose a reason for hiding this comment

sreenara commented Aug 10, 2023 • edited

InteractiveTimmy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkesavan13 commented Aug 11, 2023

1. Sync npmjs version to all of the package.json

2. Perform increment command

3. syncDependency on changed packages alone

4. Perform npm publish on changed packages alone

sreenara commented Aug 13, 2023

1. Sync npmjs version to all of the package.json

2. Perform increment command

3. syncDependency on changed packages alone

4. Perform npm publish on changed packages alone

sreenara commented Aug 14, 2023

mkesavan13 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sreenara commented Aug 14, 2023

InteractiveTimmy Aug 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sreenara commented Aug 16, 2023

InteractiveTimmy left a comment

Choose a reason for hiding this comment

sreenara commented Aug 10, 2023 •

edited

mkesavan13 left a comment •

edited

sreenara Aug 10, 2023 •

edited

sreenara commented Aug 10, 2023 •

edited

mkesavan13 left a comment •

edited

InteractiveTimmy Aug 14, 2023 •

edited