-
Notifications
You must be signed in to change notification settings - Fork 223
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
migrated jenkin build to GH action #1033
migrated jenkin build to GH action #1033
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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