-
Notifications
You must be signed in to change notification settings - Fork 438
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
Use CodeArtifact for AWS CodeBuild CI jobs #1455
Conversation
/runintegrationtests |
Looks like it worked! Executions from my CI look like:
Windows:
Logs from executions tied to this PR skip the success confirmations. |
New command correctly hides CodeArtifact URL:
Windows does not run CodeArtifact (commented out for now). |
/runintegrationtests |
else | ||
echo "CodeArtifact connection failed. Falling back to npm" | ||
fi | ||
fi |
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.
Please run this block through https://www.shellcheck.net/ . It has several issues. E.g. missing quotes around env vars.
shellcheck's SC2181 is suggesting this change:
if aws codeartifact login --tool npm --domain "$TOOLKITS_CODEARTIFACT_DOMAIN" --domain-owner "$ACCOUNT_ID" --repository "$TOOLKITS_CODEARTIFACT_REPO" > /dev/null 2>&1 ; then
echo "Connected to CodeArtifact"
else
echo "CodeArtifact connection failed. Falling back to npm"
fi
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.
Got a Powershell equivalent? 😛
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.
:) Not sure, but powershell has many fewer gotchas.
/retryBuilds |
Doing some additional testing, want to ensure that the buildscripts don't attempt to use CodeArtifact if env vars aren't present (I have confirmed that a CodePipeline tracking the child branch does use CodeArtifact correctly).ConfirmedOur main CI and Github Actions/Travis accounts are currently not wired up for CodeArtifact and should skip the CodeArtifactConfirmedpre-build
step. Tests run against this PR should confirm this.We may need to roll back Windows CodeArtifact usage. This is because we can't update our Windows builds to the newer CodeBuild image (required by CodeBuild): Extension tests are not running in a windows docker container microsoft/vscode#77499Rolled backDescription
This change implements CodeArtifact as a cache for NPM for CI jobs run by AWS CodeBuild. This gives us insurance that we can build and deploy VS Code even if NPM goes down.
Additionally, moved to
npm ci
for install to ensure only the packages in thepackage-lock
are built.Motivation and Context
We don't want to be in a place where we need a fix that we can't deploy due to issues with CI.
Testing
Tested against a private AWS account. Deploying these changes to a CodePipeline tracking the child branch resulted in a CodeArtifact repo being populated with the appropriate packages.
Screenshots (if appropriate)
Checklist
npm run newChange
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.