-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add a GitHub Actions workflow for building and publishing the extension #972
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -571,7 +571,8 @@ | |||
"build:tests": "tsc --project tsconfig.test.json", | |||
"build:plugins": "bash ./scripts/install-dev-plugins.sh", | |||
"test": "npm run build:extension && npm run build:webview && npm run build:tests && vscode-test", | |||
"preinstall": "cd ../vscode-js-debug && git submodule update --init ./ && npm install && npm exec tsc" | |||
"preinstall": "cd ../vscode-js-debug && git submodule update --init ./ && npm install && npm exec tsc", |
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.
Maybe let's add prepare:submodules
to preinstall and (cd ../vscode-js-debug && npm install && npm exec tsc)
to typecheck? wdyt?
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'm not sure when this one got added, but it'd be best not to run any submodule updates in install script. When you make changes there you don't want those changes to be accidently erased by an update cause by install. Instead, in install command we should assume submodules are already installed. Alternatively we can have a script that updates submodules and in preinstall only run a check that verifies they are there
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.
@maciekstosio I did it at first, but pulling the other submodules was too long, slowing down the CI, so I've left it as it is. But if cd ../vscode-js-debug && git submodule update --init ./
is only needed for typecheck, then it makes no sense to keep it in preinstall
step, will move it to typecheck
.
Edit: since it's needed by lint
and is a dependency to vscode-extension
, I've left the preinstall but now we ensure that the submodule won't be fetched again if already present
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.
One minor comment
.gitmodules
Outdated
@@ -1,12 +1,12 @@ | |||
[submodule "packages/simulator-server"] | |||
path = packages/simulator-server | |||
url = git@github.com:software-mansion-labs/simulator-server.git | |||
url = https://github.com/software-mansion-labs/simulator-server.git |
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.
AFAIK https links requires different type of setup locally when you want the repositories to be writeable. This means that while it may be read on CI it will cause problems on dev installations where people wouldn't be able to make updates directly from submodules (unless they set up HTTPS properly on their computers which I don't know how can be done).
I'd prefer if we could keep this file unchanged assuming there's some easy easy way to make it work. I quickly found that you can pass submodules
option when loading main repo: https://github.com/marketplace/actions/checkout-submodules#alternatives
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 problem is it doesn't work with SSH without any additional setup, passing submodules
has to be ruled out as simulator-server
is private and we don't want to pull it, as well as the other, non-essential for CI submodules that take too long to pull.
Edit: found a workaround with setting git config
to use HTTPS on CI
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.
can we list the submodules we want to checkout in the action then? There's another example under the link I sent that does this:
steps:
- name: Checkout repository
uses: actions/checkout@v2
- name: Checkout submodules
run: git submodule update --init --recursive
So here instead of running wildcard update we can list submodules we want to pull
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.
^ This works only with HTTPS modules, if we want to keep SSH we need some shenanigans.
I think it's better to keep the submodules we need to check out in one place (npm scripts). In 33b4e2c, I've added a workaround with setting git config to use HTTPS on CI and that resolves the issue with SSH submodules.
@@ -571,7 +571,8 @@ | |||
"build:tests": "tsc --project tsconfig.test.json", | |||
"build:plugins": "bash ./scripts/install-dev-plugins.sh", | |||
"test": "npm run build:extension && npm run build:webview && npm run build:tests && vscode-test", | |||
"preinstall": "cd ../vscode-js-debug && git submodule update --init ./ && npm install && npm exec tsc" | |||
"preinstall": "cd ../vscode-js-debug && git submodule update --init ./ && npm install && npm exec tsc", |
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'm not sure when this one got added, but it'd be best not to run any submodule updates in install script. When you make changes there you don't want those changes to be accidently erased by an update cause by install. Instead, in install command we should assume submodules are already installed. Alternatively we can have a script that updates submodules and in preinstall only run a check that verifies they are there
echo "Downloading simulator-server assets for tag $sim_server_tag" | ||
|
||
# List release assets using GitHub API | ||
release_info=$(curl -s "https://api.github.com/repos/software-mansion-labs/simulator-server-releases/releases/$sim_server_tag") |
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 just typed https://api.github.com/repos/software-mansion-labs/simulator-server-releases/releases/latest
into the url bar and got hit with a rate limit:
{
"message": "API rate limit exceeded for 83.142.186.98. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
"documentation_url": "https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"
}
Is there a way to include that authentication they are talking about in the scripts for us, or otherwise to try and use old release method and only fall back to this one if the user does not have authenticated gh cli?
other wise it seems very easy to block ourselves from accessing published versions of sim-server. I also don't know if it is a git repository specific rate limit or whole api wide, caused by other projects across the company (I'm at the office) but if it's global it might be worth mitigating additional stress on the API
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.
There's so many scripts already that I'd rather keep it simple. I'll probably restore the GH CLI approach and use secrets.GITHUB_TOKEN
for CI authentication.
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 left an inline comment
echo "Downloading simulator-server assets for tag $sim_server_tag" | ||
|
||
# List release assets using GitHub API | ||
release_info=$(curl -s "https://api.github.com/repos/software-mansion-labs/simulator-server-releases/releases/$sim_server_tag") |
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.
are these going to be rate limited? was wondering if we should perhaps stick to using github via CLI where we can authenticate requests with GITHUB_TOKEN or use GITHUB_TOKEN
here to make the curl request
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 didn't encounter any rate limits, but since it happened to @filip131311 then yeah, It makes sense to restore GH CLI and use GITHUB_TOKEN
for authentication.
8c2d39e
to
b98dc86
Compare
b98dc86
to
ae8bf79
Compare
ae8bf79
to
667d552
Compare
667d552
to
999ff18
Compare
999ff18
to
33b4e2c
Compare
In 33b4e2c I've made changes ensuring that submodules are not overwritten and updated only if not present, switched back to SSH submodules with a workaround for using HTTPS on CI, reverted sim server assets downloader to use GH CLI and tested packaging on CI – the new workflow is working (the only thing left out to test is publishing itself, but first we need a new release) |
Fixes: #976
This PR:
simulator-server
repo,git config
,init-submodules-if-not-present
script to fetch submodules when needed (lint
,typecheck
,build:dist
,build:extension-debug
),How Has This Been Tested:
Tested packaging locally on a fresh repo and on CI, the only thing left is the publishing itself.