Bump node version, add tests and code coverage by garrytrinder · Pull Request #26 · pnp/action-cli-deploy

garrytrinder · 2023-08-21T20:47:35Z

Merging this PR will

Bump action to node@16
- Causes major version bump to v4.0.0(see https://github.blog/changelog/2022-05-20-actions-can-now-run-in-a-node-js-16-runtime/)
Update contributors
Add mocha unit tests
Add c8 code coverage
Add support for Mocha Test Explorer VS Code extension
Add test workflows
Add PR check workflow
Update dependencies
Fix dependency versions
Update contributors
Remove .DS_Store file
- Add to .gitignore
Update readme
- Update compatibility matrix
- Remove release notes
Remove tslint
Add eslint rules
- Two space Indenting
- Single quote rule, quote literals allowed
Update npm scripts

Jwaegebaert

Awesome work with this PR @garrytrinder. I'm no master of GitHub actions but at first glance, they look all right. I've left a few suggestions but those are mostly cosmetic 😉

Jwaegebaert · 2023-08-23T12:57:26Z

@@ -1,23 +1,30 @@
-name: 'CLI for Microsoft 365 Deploy App'
-description: 'Deploy an app using CLI for Microsoft 365'
+name: "CLI for Microsoft 365 Deploy App"


I would prefer to use single quotes instead of double quotes as we also use single quotes in the CLI 😄
But this is a personal preference of course.

Jwaegebaert · 2023-08-23T12:59:15Z

+  "keywords": [
+    "office 365",
+    "microsoft 365",
+    "sharepoint framework",
+    "o365",
+    "m365",
+    "spfx",
+    "sharepoint online",
+    "sharepoint",
+    "microsoft teams",
+    "microsoft graph",
+    "microsoft flow",
+    "azure active directory",
+    "azure ad",
+    "azure",
+    "microsoft"
+  ],


Maybe a few additional keywords regarding power platform could be handy here

@Jwaegebaert good catch, these are the same keywords we have in the main CLI repo, so we should consider also adding power platform there as well.

I think these keywords need a review. Let's leave them as they are for now and work on a new set of keywords that we can apply across all of our repos.

Good idea, I'll create an issue in the repo of the CLI with a suggestion and we can discuss it there.

Jwaegebaert · 2023-08-23T13:26:47Z

+describe('validate', () => {
+
+    it('throws error if APP_FILE_PATH is not passed ', () => {


Suggested change

describe('validate', () => {

it('throws error if APP_FILE_PATH is not passed ', () => {

describe('validate', () => {

it('throws error if APP_FILE_PATH is not passed ', () => {

Jwaegebaert · 2023-08-23T13:27:17Z

+            SKIP_FEATURE_DEPLOYMENT: false,
+            OVERWRITE: false
+        };
+        assert.throws(() => validate(options), Error, `SCOPE must be either 'tenant' or Scope.SiteCollection.`);


Suggested change

assert.throws(() => validate(options), Error, `SCOPE must be either 'tenant' or Scope.SiteCollection.`);

assert.throws(() => validate(options), Error, 'SCOPE must be either 'tenant' or Scope.SiteCollection.');

Jwaegebaert · 2023-08-23T13:27:27Z

+            SKIP_FEATURE_DEPLOYMENT: false,
+            OVERWRITE: false
+        };
+        assert.throws(() => validate(options), Error, `SITE_COLLECTION_URL is required if SCOPE is set to Scope.SiteCollection.`);


Suggested change

assert.throws(() => validate(options), Error, `SITE_COLLECTION_URL is required if SCOPE is set to Scope.SiteCollection.`);

assert.throws(() => validate(options), Error, 'SITE_COLLECTION_URL is required if SCOPE is set to Scope.SiteCollection.');

garrytrinder · 2023-08-25T21:28:17Z

Thanks for the review comments @Jwaegebaert 👍

I've addressed the indenting and quoting comments by applying eslint rules so we fix the style going forwards.

I've taken the approach of using no quotes > single quotes > double quotes for the action.yml file. This seems to be the best approach based on a little research, (see https://stackoverflow.com/a/69850618).

I've also updated some of the npm scripts to bring them inline with the main CLI repo so that we have a more consistent developer experience.

Jwaegebaert

Awesome idea to add the eslint rules. I noticed a few files still have a spacing of 4 instead of 2, but I commented on them. I also found another nice addition we could apply. Besides these, it looks good to go 😄

Jwaegebaert · 2023-08-26T17:28:41Z

+      - name: Use Node.js ${{ matrix.node }}
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.node }}
+          registry-url: "https://registry.npmjs.org"
+      - name: Cache node modules
+        id: cache
+        uses: actions/cache@v3
+        with:
+          path: |
+            **/node_modules
+          key: node_modules-${{ matrix.os }}-${{ matrix.node }}-${{ hashFiles('**/npm-shrinkwrap.json') }}
+      - name: Restore dependencies
+        if: steps.cache.outputs.cache-hit != 'true'
+        run: npm ci
+      - name: Build
+        run: npm run build
+      - name: Compress output (non-Windows)
+        if: matrix.os != 'windows-latest'
+        run: tar -cvf build.tar --exclude node_modules ./
+      - name: Compress output (Windows)
+        if: matrix.os == 'windows-latest'
+        run: 7z a -ttar -xr!node_modules -r build.tar .
+      - name: Upload build artifact
+        uses: actions/upload-artifact@v3
+        with:
+          name: build-${{ matrix.os }}-${{ matrix.node }}
+          path: build.tar


In .github/workflows/test.yml we have an empty line before a new task which looks quite neat. So a suggestion would be to apply it here as well 😄

Suggested change

- name: Use Node.js ${{ matrix.node }}

uses: actions/setup-node@v3

with:

node-version: ${{ matrix.node }}

registry-url: "https://registry.npmjs.org"

- name: Cache node modules

id: cache

uses: actions/cache@v3

with:

path: |

**/node_modules

key: node_modules-${{ matrix.os }}-${{ matrix.node }}-${{ hashFiles('**/npm-shrinkwrap.json') }}

- name: Restore dependencies

if: steps.cache.outputs.cache-hit != 'true'

run: npm ci

- name: Build

run: npm run build

- name: Compress output (non-Windows)

if: matrix.os != 'windows-latest'

run: tar -cvf build.tar --exclude node_modules ./

- name: Compress output (Windows)

if: matrix.os == 'windows-latest'

run: 7z a -ttar -xr!node_modules -r build.tar .

- name: Upload build artifact

uses: actions/upload-artifact@v3

with:

name: build-${{ matrix.os }}-${{ matrix.node }}

path: build.tar

- name: Use Node.js ${{ matrix.node }}

uses: actions/setup-node@v3

with:

node-version: ${{ matrix.node }}

registry-url: "https://registry.npmjs.org"

- name: Cache node modules

id: cache

uses: actions/cache@v3

with:

path: |

**/node_modules

key: node_modules-${{ matrix.os }}-${{ matrix.node }}-${{ hashFiles('**/npm-shrinkwrap.json') }}

- name: Restore dependencies

if: steps.cache.outputs.cache-hit != 'true'

run: npm ci

- name: Build

run: npm run build

- name: Compress output (non-Windows)

if: matrix.os != 'windows-latest'

run: tar -cvf build.tar --exclude node_modules ./

- name: Compress output (Windows)

if: matrix.os == 'windows-latest'

run: 7z a -ttar -xr!node_modules -r build.tar .

- name: Upload build artifact

uses: actions/upload-artifact@v3

with:

name: build-${{ matrix.os }}-${{ matrix.node }}

path: build.tar

Jwaegebaert · 2023-08-26T17:29:52Z

+      - name: Configure pagefile
+        if: matrix.os == 'windows-latest'
+        uses: al-cheb/[email protected]
+        with:
+          minimum-size: 16GB
+          maximum-size: 16GB
+          disk-root: "C:"
+      - uses: actions/download-artifact@v3
+        with:
+          name: build-${{ matrix.os }}-${{ matrix.nodeBuild }}
+      - name: Unpack build artifact (non-Windows)
+        if: matrix.os != 'windows-latest'
+        run: tar -xvf build.tar && rm build.tar
+      - name: Unpack build artifact (Windows)
+        if: matrix.os == 'windows-latest'
+        run: 7z x build.tar && del build.tar
+      - name: Use Node.js ${{ matrix.nodeRun }}
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.nodeRun }}
+          registry-url: "https://registry.npmjs.org"
+      - name: Cache node modules
+        id: cache
+        uses: actions/cache@v3
+        with:
+          path: |
+            **/node_modules
+          key: node_modules-${{ matrix.os }}-${{ matrix.nodeBuild }}-${{ hashFiles('**/npm-shrinkwrap.json') }}
+      - name: Restore dependencies
+        if: steps.cache.outputs.cache-hit != 'true'
+        run: npm ci
+      - name: Test without coverage
+        run: npm test
+      - name: Test with coverage
+        run: npm run test:cov
+      - uses: actions/upload-artifact@v3
+        with:
+          name: coverage-${{ matrix.os }}-${{ matrix.nodeRun }}
+          path: coverage.tar


Let's add an empty line before every task.

Suggested change

- name: Configure pagefile

if: matrix.os == 'windows-latest'

uses: al-cheb/[email protected]

with:

minimum-size: 16GB

maximum-size: 16GB

disk-root: "C:"

- uses: actions/download-artifact@v3

with:

name: build-${{ matrix.os }}-${{ matrix.nodeBuild }}

- name: Unpack build artifact (non-Windows)

if: matrix.os != 'windows-latest'

run: tar -xvf build.tar && rm build.tar

- name: Unpack build artifact (Windows)

if: matrix.os == 'windows-latest'

run: 7z x build.tar && del build.tar

- name: Use Node.js ${{ matrix.nodeRun }}

uses: actions/setup-node@v3

with:

node-version: ${{ matrix.nodeRun }}

registry-url: "https://registry.npmjs.org"

- name: Cache node modules

id: cache

uses: actions/cache@v3

with:

path: |

**/node_modules

key: node_modules-${{ matrix.os }}-${{ matrix.nodeBuild }}-${{ hashFiles('**/npm-shrinkwrap.json') }}

- name: Restore dependencies

if: steps.cache.outputs.cache-hit != 'true'

run: npm ci

- name: Test without coverage

run: npm test

- name: Test with coverage

run: npm run test:cov

- uses: actions/upload-artifact@v3

with:

name: coverage-${{ matrix.os }}-${{ matrix.nodeRun }}

path: coverage.tar

- name: Configure pagefile

if: matrix.os == 'windows-latest'

uses: al-cheb/[email protected]

with:

minimum-size: 16GB

maximum-size: 16GB

disk-root: "C:"

- uses: actions/download-artifact@v3

with:

name: build-${{ matrix.os }}-${{ matrix.nodeBuild }}

- name: Unpack build artifact (non-Windows)

if: matrix.os != 'windows-latest'

run: tar -xvf build.tar && rm build.tar

- name: Unpack build artifact (Windows)

if: matrix.os == 'windows-latest'

run: 7z x build.tar && del build.tar

- name: Use Node.js ${{ matrix.nodeRun }}

uses: actions/setup-node@v3

with:

node-version: ${{ matrix.nodeRun }}

registry-url: "https://registry.npmjs.org"

- name: Cache node modules

id: cache

uses: actions/cache@v3

with:

path: |

**/node_modules

key: node_modules-${{ matrix.os }}-${{ matrix.nodeBuild }}-${{ hashFiles('**/npm-shrinkwrap.json') }}

- name: Restore dependencies

if: steps.cache.outputs.cache-hit != 'true'

run: npm ci

- name: Test without coverage

run: npm test

- name: Test with coverage

run: npm run test:cov

- uses: actions/upload-artifact@v3

with:

name: coverage-${{ matrix.os }}-${{ matrix.nodeRun }}

path: coverage.tar

Jwaegebaert · 2023-08-26T17:31:11Z

@@ -0,0 +1,35 @@
+{
+    "root": true,


Here we also have a spacing of 4 instead of 2

Jwaegebaert · 2023-08-26T17:32:10Z

@@ -0,0 +1,24 @@
+export interface SpoApp {
+    CheckInComment: string,


Here we also have a spacing of 4 instead of 2

Jwaegebaert · 2023-08-26T17:33:04Z

+export interface Options {
+    APP_FILE_PATH: string;
+    SCOPE: Scope;
+    SITE_COLLECTION_URL: string;
+    SKIP_FEATURE_DEPLOYMENT: boolean;
+    OVERWRITE: boolean;
+}
+
+export enum Scope {
+    Tenant = 'tenant',
+    SiteCollection = 'sitecollection'
+}


Here we also have a spacing of 4 instead of 2

Suggested change

export interface Options {

APP_FILE_PATH: string;

SCOPE: Scope;

SITE_COLLECTION_URL: string;

SKIP_FEATURE_DEPLOYMENT: boolean;

OVERWRITE: boolean;

}

export enum Scope {

Tenant = 'tenant',

SiteCollection = 'sitecollection'

}

export interface Options {

APP_FILE_PATH: string;

SCOPE: Scope;

SITE_COLLECTION_URL: string;

SKIP_FEATURE_DEPLOYMENT: boolean;

OVERWRITE: boolean;

}

export enum Scope {

Tenant = 'tenant',

SiteCollection = 'sitecollection'

}

Jwaegebaert · 2023-08-26T17:35:04Z

@@ -0,0 +1,11 @@
+{
+    "extension": [


Here we also have a spacing of 4 instead of 2

Jwaegebaert

Great job on this one, @garrytrinder! I don't have anything else to add to this PR, so it's a go from me 😄

garrytrinder · 2023-08-29T04:54:48Z

Thanks again @Jwaegebaert 👍 I managed to fix the eslint rules so that we can now detect errors in the interfaces and enums which were previously being ignored. Thanks again for your attention to detail ❤️

garrytrinder requested review from milanholemans and removed request for milanholemans August 21, 2023 20:48

garrytrinder force-pushed the node-bump branch 3 times, most recently from 73cef77 to 148dc84 Compare August 22, 2023 20:34

Jwaegebaert requested changes Aug 23, 2023

View reviewed changes

garrytrinder force-pushed the node-bump branch from 148dc84 to 4e35179 Compare August 25, 2023 21:19

Jwaegebaert requested changes Aug 26, 2023

View reviewed changes

Jwaegebaert approved these changes Aug 28, 2023

View reviewed changes

garrytrinder force-pushed the node-bump branch from 816f228 to d2dcbb6 Compare August 28, 2023 20:56

Bump node version, add tests and code coverage

fd83463

garrytrinder force-pushed the node-bump branch from d2dcbb6 to fd83463 Compare August 29, 2023 04:56

garrytrinder merged commit 1bc9bd9 into pnp:master Aug 29, 2023

garrytrinder deleted the node-bump branch August 29, 2023 04:57

garrytrinder mentioned this pull request Aug 29, 2023

Maintenance Updates #17

Closed

18 tasks

Jwaegebaert mentioned this pull request Aug 30, 2023

Update package keywords pnp/cli-microsoft365#5433

Closed

		describe('validate', () => {

		it('throws error if APP_FILE_PATH is not passed ', () => {

	assert.throws(() => validate(options), Error, `SCOPE must be either 'tenant' or Scope.SiteCollection.`);
	assert.throws(() => validate(options), Error, 'SCOPE must be either 'tenant' or Scope.SiteCollection.');

		@@ -0,0 +1,24 @@
		export interface SpoApp {
		CheckInComment: string,

Conversation

garrytrinder commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jwaegebaert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

garrytrinder commented Aug 25, 2023

Uh oh!

Jwaegebaert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jwaegebaert left a comment

Choose a reason for hiding this comment

Uh oh!

garrytrinder commented Aug 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

garrytrinder commented Aug 21, 2023 •

edited

Loading