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

Integrate pull script with CDK. #103

Merged
merged 8 commits into from
Mar 22, 2021

Conversation

bryce-shang
Copy link
Contributor

@bryce-shang bryce-shang commented Mar 2, 2021

Issues:

Addresses CryptoAlg-663

Description of changes:

This PR integrated the script created in #102 with CDK code.

  • The new change assumes CodeBuild and SSM runCommand result success status can indicate all images are pushed to ECR instead of checking images with aws ecr.
  • To provide the script with GitHub access token, the major change of this PR is to store the token in SecretsManager.
  • To pass the token to run-cdk.sh script, arguments parser are refactored.

Call-outs:

Testing:

See log in CryptoAlg-663?selectedConversation=33d3e3af-a5b5-4e5d-b78f-b4f49b8e250a

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bryce-shang bryce-shang marked this pull request as draft March 2, 2021 21:52
@bryce-shang bryce-shang force-pushed the integrate-pull-script branch from e7d9b51 to aa2e673 Compare March 2, 2021 22:08
@bryce-shang bryce-shang force-pushed the integrate-pull-script branch from aa2e673 to c044042 Compare March 3, 2021 00:00
@bryce-shang bryce-shang marked this pull request as ready for review March 8, 2021 18:26
```
$ ./run-cdk.sh {aws-account-id} {region} {github-repo-owner} {github-repo-name} {ACTION}
./run-cdk.sh --action deploy-ci --github-access-token 123456
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that --github-access-token is something that the user provides? If so, I think it would make it more clear to have them set an environment variable with their token and then use that. Aren't we already using an environment variable somewhere with the user's personal github access token?

export GITHUB_ACCESS_TOKEN=...
./run-cdk.sh --action deploy-ci --github-access-token ${GITHUB_ACCESS_TOKEN}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, to connect AWS CodeBuild with GitHub, personal GitHub access token was used. Now, it's replaced with GitHub OAuth. Besides, this new access token needs read:package permission, which is not needed during CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the env variable.

```
./run-cdk.sh 123456789 us-west-2 GitHubUserName aws-lc SYNTH
./run-cdk.sh --action build-img --github-access-token 123456
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the env variable.

export S3_FOR_WIN_DOCKER_IMG_BUILD="${AWS_LC_S3_BUCKET_PREFIX}-${DATE_NOW}"
export WIN_EC2_TAG_KEY="aws-lc"
export WIN_EC2_TAG_KEY='aws-lc'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we change this to single quotes? Was this being interpreted weirdly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the change is to use double quotes on demand.

tests/ci/docker_images/linux-x86/README.md Show resolved Hide resolved
tests/ci/cdk/run-cdk.sh Outdated Show resolved Hide resolved
tests/ci/cdk/run-cdk.sh Outdated Show resolved Hide resolved
@bryce-shang bryce-shang requested a review from nebeid March 19, 2021 22:29
@bryce-shang bryce-shang merged commit fa1ee9f into aws:aws-c-cal-ci Mar 22, 2021
bryce-shang added a commit that referenced this pull request Mar 25, 2021
* Pre-pull aws crt Docker images.

* Remove unused code.

* Run pull_github_pkg.sh in CodeBuild.

* Update files under 'generated-src' dir.

* Pre-pull aws crt Docker images. (#102)

* Pre-pull aws crt Docker images.

* Remove unused code.

* Address comments.

* Move common source.

* Integrate pull script with CDK. (#103)

* Pre-pull aws crt Docker images.

* Remove unused code.

* Run pull_github_pkg.sh in CodeBuild.

* Address comments.

* Move common source.

* Change README.md

* Address comments.

* Build and test without Perl/Go. (#107)

* Build and test without Perl/Go.

* Revert 'delete update-webhook cli'

* Fix typo.

* Adds build flavor -DBUILD_TESTING=0 -DBUILD_SSL=0.

* Adds build flavor -DBUILD_TESTING=0 -DBUILD_SSL=0 with release mode.

* Fix typo.

* Remove 'AWS_LC' prefix.

* Fix build flag.

* Add note.

* Remove build with -DBUILD_TESTING=0 -DBUILD_SSL=0.

* Remove build flags - BUILD_WITHOUT_GO and BUILD_WITHOUT_PERL..
@bryce-shang bryce-shang deleted the integrate-pull-script branch May 12, 2021 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants