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

New command: spfx project github workflow add #5209

Closed
Adam-it opened this issue Jul 7, 2023 · 22 comments
Closed

New command: spfx project github workflow add #5209

Adam-it opened this issue Jul 7, 2023 · 22 comments

Comments

@Adam-it
Copy link
Contributor

Adam-it commented Jul 7, 2023

Usage

m365 spfx project github workflow add [options]

Description

Adds a GitHub workflow for a SharePoint Framework project.

Options

Option Description
-n, --name [name] Name of the workflow that will be created. If none is specified a default name will be used 'Deploy Solution ${name of sppkg file}'
-b, --branchName [branchName] Specify the branch name which should trigger the workflow on push. If none is specified a default will be used which is 'main'
-m, --manuallyTrigger When specified a manual trigger option will be added to the workflow: workflow_dispatch
-l, --loginMethod [loginMethod] Specify the login method used for the login action. Possible options are: user, application. Default application
-s, --scope [scope] cope of the app catalog: tenant, sitecollection. Default is tenant
--siteUrl [siteUrl] The URL of the site collection where the solution package will be added. Required if scope is set to sitecollection
--skipFeatureDeployment When specified and the app supports tenant-wide deployment, deploy it to the whole tenant.
--overwrite set to overwrite the existing package file.

Examples

Adds a GitHub workflow for a SharePoint Framework project with application login method triggered on push to main

m365 spfx project github workflow add 

Adds a GitHub workflow for a SharePoint Framework project with user login method triggered on push to main and when manually triggered.

m365 spfx project github workflow add --manuallyTrigger --loginMethod "user"

Adds a GitHub workflow for a SharePoint Framework project with deployment to a site collection app catalog

m365 spfx project github workflow add --scope "sitecollection" --siteUrl "https://some.sharepoint.com/sites/someSite"

Default properties

none

Additional Info

Remarks

When loginMethod option is used with user value then the CLI for Microsoft 365 login action will be created with ADMIN_USERNAME and ADMIN_PASSWORD. If the application value will be used then the action will created with CERTIFICATE_ENCODED, CERTIFICATE_PASSWORD, APP_ID.

Example workflow

This is an example workflow that should be generated (without the comments 😉)

name: Deploy Solution # {{SOLUTION_NAME}} # default title of flow with possibility to change it with an option in the command

on:
  push:
    branches:
      - main # optionally specify name of the branch which should trigger the flow by specifying an option in the command. by default it is main
  workflow_dispatch: # specify that the flow can be triggered manually by specifying a flag in the command

jobs:
  build-and-deploy:
    runs-on: ubuntu-latest
    
    steps:

    - name: Checkout
      uses: actions/checkout@v3.5.3
      
    - name: Use Node.js 16.x
      uses: actions/setup-node@v3.7.0
      with:
        node-version: 16.x
    
    - name: Run npm ci
      run: npm ci
    
    - name: Bundle & Package
      run: |
        gulp bundle --ship
        gulp package-solution --ship
    
    - name: CLI for Microsoft 365 Login
      uses: pnp/action-cli-login@v2.2.2
      with:
        #ADMIN_USERNAME:  ${{ secrets.ADMIN_USERNAME }} # this will be used when `user` method is specified
        #ADMIN_PASSWORD:  ${{ secrets.ADMIN_PASSWORD }} # this will be used when `user` method is specified
        CERTIFICATE_ENCODED: ${{ secrets.CERTIFICATE_ENCODED }} # this will be used when `application` method is specified
        CERTIFICATE_PASSWORD: # this will be used when `application` method is specified
        APP_ID: ${{ secrets.APP_ID }} # this will be used when `application` method is specified
    
    - name: CLI for Microsoft 365 Deploy App
      uses: pnp/action-cli-deploy@v3.0.1
      with:
        APP_FILE_PATH: sharepoint/solution/{{ solutionName }}.sppkg # should be automatically populated based on the project
        #SCOPE: # Scope of the app catalog: tenant or sitecollection. Default is tenant by specifying an option in the command
        #SITE_COLLECTION_URL: # The URL of the site collection where the solution package will be added. Required if scope is set to sitecollection by specifying an option in the command
        SKIP_FEATURE_DEPLOYMENT: false # If the app supports tenant-wide deployment, deploy it to the whole tenant by specifying an option in the command. Default is false
        OVERWRITE: true # Set to overwrite the existing package file by specifying an option in the command. Default is false
...

Implementation idea

How I would go about it is maybe keep a .yml file as part of the project in the command folder and just adjust it based on the options passed. We could have some markers inside the template .yml file and just use plain replace based on what was specified

File

The command (like other spfx commands) should work within and SPFx project directory. On the root level of the SPFx project (so the same directory where package.json is present) the command should create a file with name deploy.yml and it should be created in .github/workflow/ subdirectories (the command should create those folders if not present).
When a file with the same name already exist the command should present an error message.

Some other ideas

👉 I wasn't sure if it will be useful but CLI for Microsoft 365 actions also support specifying a CLI version that will be used and we may specify latest, next or a specific version tag.
👉 Also not sure if giving the possibility to specify a specific node version that will be used might be helpful. The latest version of SPFx supports v16.x but maybe someone would like to create a workflow for an older version of SPFx 🤔
👉 Another idea for an option would be if the bundle and package should be with --ship o not. Maybe someone would like a workflow that deploys a SPFx app for debugging

@Adam-it Adam-it added new feature needs peer review Needs second pair of eyes to review the spec or PR labels Jul 7, 2023
@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 7, 2023

@pnp/cli-for-microsoft-365-maintainers what do you think?
BTW I would like to start working on it

@waldekmastykarz
Copy link
Member

Great idea! Few considerations:

  • I suggest that we use application login as default. It's safer than a user and doesn't require the organization to keep around an admin-level account without MFA
  • I suggest we start simple and leave the Node and CLI versions out. We can always add them later if need be
  • I like the idea of having a template for the yml file and replacing markers. Alternatively, we could look into defining the template in an object and see if there's a package capable of turning a JS object into yaml which would be less error prone

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 8, 2023

  • I suggest that we use application login as default. It's safer than a user and doesn't require the organization to keep around an admin-level account without MFA

I agree, will update the spec

  • I suggest we start simple and leave the Node and CLI versions out. We can always add them later if need be

I thought exactly the same, that's why it is not in the spec but as a suggestion. Ok for now let's leave it

  • I like the idea of having a template for the yml file and replacing markers. Alternatively, we could look into defining the template in an object and see if there's a package capable of turning a JS object into yaml which would be less error prone

Awesome idea. Will do some research in that area.

@waldekmastykarz thanks for the feedback 👍

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 8, 2023

Ok, @waldekmastykarz I updated the spec.
@pnp/cli-for-microsoft-365-maintainers any other comments?

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 8, 2023

@waldekmastykarz this looks promising js-yaml

https://github.com/nodeca/js-yaml#readme

Allows to convert both ways from object to yaml and the other way around. Which might be helpful if we want to update the workflow template in the future. We could keep the object JSON as the workflow template.
The package is:
-Used by couple of million users
-has some opened issues and PRs
-has 800 forks
-it has 60 m weekly downloads https://www.npmjs.com/package/js-yaml
-last time it was published 2 years ago 🤔

What do you think? We could at least try with this and check if it will work. We also should double check for security issues.

@pnp/cli-for-microsoft-365-maintainers any alternative suggestions?

@waldekmastykarz
Copy link
Member

Lack of activity on the repo is concerning. I suggest that we look for alternatives.

@waldekmastykarz
Copy link
Member

Spec looks good. One detail we miss is the name of the file to create on disk and what the command will do in case of a collision.

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 8, 2023

Ok will research something more

Spec looks good. One detail we miss is the name of the file to create on disk and what the command will do in case of a collision.

Those are good questions. Usually when I was working with pipelines the filename a bit described what was going to happen. Like 'release_main.yml' means that it will release the product from main branch.
In this case maybe we could just name it deploy.yml. what do you think? In case of collisions I would show an error message. We could also think of adding the --force to overwrite the file in case one already exists.
What do you think?

@martinlingstuyl
Copy link
Contributor

So @Adam-it, so this command will generate a yml file for you in the directory where you are running it, so you can commit it to the repo and make GH run it. Is that the idea?

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 8, 2023

So @Adam-it, so this command will generate a yml file for you in the directory where you are running it, so you can commit it to the repo and make GH run it. Is that the idea?

I would suggest that the command should run within a SPFx project directory, similar to other spfx commands.
The aim would be to generate the deploy.yml file in the project root directory, so the same folder where package.json is present. The workflow should be created in \.github\workflows subdirectories, as this is the default way GitHub handles (looks for) workflow files. I think that if those folders are not present the command needs to create them along the way.

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 8, 2023

@martinlingstuyl, @waldekmastykarz
I updated the spec with answers to your questions:

  • filename,
  • what should be done in case of collision
  • .yml file location

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 8, 2023

Lack of activity on the repo is concerning. I suggest that we look for alternatives.

ok second option I would recommend (I think even better then the first one) is yaml
https://www.npmjs.com/package/yaml
https://github.com/eemeli/yaml

  • 951 stars
  • actively maintained. Last commit is from 26 May
  • used by millions
  • npm package has millions of downloads

what do you think?

@martinlingstuyl
Copy link
Contributor

Ok, great, can we change the description a bit to make it a bit clearer?

@martinlingstuyl
Copy link
Contributor

The docs for this command can also benefit from a good remarks section, I think...

@martinlingstuyl
Copy link
Contributor

ok second option I would recommend (I think even better then the first one) is yaml
https://www.npmjs.com/package/yaml
https://github.com/eemeli/yaml

Seems a good choice!

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 9, 2023

Ok, great, can we change the description a bit to make it a bit clearer?

Absolutely, what would you add?

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 9, 2023

The docs for this command can also benefit from a good remarks section, I think...

Sure, what would you had in mind?
Our command issue template does not have a remarks section that's why it is quite small and added at the end

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 9, 2023

Ok seems I got 2x✅ from @waldekmastykarz and @martinlingstuyl, right? The only things we now consider is more clarity in the spec or detail 🤔.
In that case I will start working on this one

@Adam-it Adam-it self-assigned this Jul 9, 2023
@Adam-it Adam-it added work in progress and removed needs peer review Needs second pair of eyes to review the spec or PR labels Jul 9, 2023
@waldekmastykarz
Copy link
Member

The yaml package seems good on the surface: active, a group of contributors, regular releases. Let's try it! Good find!

@martinlingstuyl
Copy link
Contributor

martinlingstuyl commented Jul 9, 2023

About the usage @Adam-it:

We could also make it less specific:

m365 spfx project pipeline add --type GitHub --etc

We could later add extra types, like AzureDevOps...

Just a brainwave

@Adam-it
Copy link
Contributor Author

Adam-it commented Jul 9, 2023

About the usage @Adam-it:

We could also make it less specific:

m365 spfx project pipeline add --type GitHub --etc

We could later add extra types, like AzureDevOps...

Just a brainwave

This was actually made up on basecamp. The decision here is to support GH only for now as our CLI actions will only work for GH. As for Azure DevOps it has to be handled differently which actually would resolve in a totally different set of options. Due to that fact this command would be really hard to maintain, large, and would consist of lot of if else code blocks.
I think what @waldekmastykarz wrote on basecamp is the correct way. For now let's focus on GH as this is what we have with actions. And I think it would be wiser to have a separate command for Azure DevOps.

Also pipeline I guess is more used in DevOps. In GH this is classified as workflow. So the name would also need to be different I guess 🤔. Maybe something like cicd flow

All in all I think it is a good suggestion to consider other solutions than just GH. But I am not sure if doing all in one command would be the way to go 🤔. At least in this particular case.

@waldekmastykarz
Copy link
Member

I suggest that we use multiple commands, at least to start. Should we expand in the future, we can always revisit our stance. Since each CI system has different format of pipes, putting the different systems into one command could lead to too much unrelated code put into one command, not to mention different options required by each format. I suggest we start with GH and if we add other systems in the future, see if it makes sense to combine them or to keep them separate.

@milanholemans milanholemans added this to the v6.11 milestone Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants