Conversation
.github/workflows/release.yml
Outdated
| - name: Package | ||
| run: | | ||
| node ./out/build/update-readme.js | ||
| vsce package -o vscode-yaml-${{ env.EXT_VERSION }}-${GITHUB_RUN_NUMBER}-${target}.vsix |
There was a problem hiding this comment.
${target} isn't defined anywhere, because we package a single vsix everywhere. It's safe to remove the reference.
build/update-readme.ts
Outdated
|
|
||
| const lines = `${readme}`.split('\n'); | ||
|
|
||
| const index = lines.findIndex((line) => line.includes('## Overview')); |
There was a problem hiding this comment.
The README doesn't contain the heading "Overview", so this does nothing. I think it's worth removing this build script, as well as line 60 in the GitHub Action which calls this script.
There was a problem hiding this comment.
In fact node ./out/build/update-readme.js fails for me with Error: Cannot find module '/home/runner/work/vscode-yaml/vscode-yaml/out/build/update-readme.js'
| publishPreRelease: | ||
| description: 'Publish a pre-release ?' | ||
| required: true | ||
| type: choice | ||
| options: | ||
| - 'true' | ||
| - 'false' | ||
| default: 'false' |
There was a problem hiding this comment.
I would remove this. We don't support pre-releases here either, and the logic to do it is missing anyways.
There was a problem hiding this comment.
We did support prerelease YAML in Jenkins earlier
.github/workflows/release.yml
Outdated
| - name: Publish to VS Code Marketplace | ||
| if: ${{ github.event_name == 'schedule' || inputs.publishToMarketPlace == 'true' || inputs.publishPreRelease == 'true' }} | ||
| run: | | ||
| vsce publish -p ${{ secrets.VSCODE_MARKETPLACE_TOKEN }} --packagePath vscode-yaml/vscode-yaml-${{ env.EXT_VERSION }}-${GITHUB_RUN_NUMBER}-${platform}.vsix |
There was a problem hiding this comment.
${platform} also isn't defined anywhere so it can be removed here and below.
.github/workflows/release.yml
Outdated
| - name: Install dependencies | ||
| run: | | ||
| npm install -g typescript "yarn" "@vscode/vsce" "ovsx" | ||
| npm install |
There was a problem hiding this comment.
Given this project uses yarn, we should use yarn to install dependencies. In the next step, on line 55, you run yarn install, so I think it should be okay to remove this line (just line 45, we still need line 44).
datho7561
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you, Muthu!
What does this PR do?
Migrated Jenkin build into GH action
What issues does this PR fix or reference?
#1031
Is it tested? How?
NA