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

ci: rc, release, tagged rc, hotfix workflows #12918

Merged
merged 22 commits into from
Aug 3, 2023

Conversation

awsluja
Copy link
Contributor

@awsluja awsluja commented Jul 7, 2023

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Pull request labels are added

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

@awsluja awsluja requested review from a team as code owners July 7, 2023 20:43
Copy link
Member

@sobolk sobolk left a comment

Choose a reason for hiding this comment

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

it seems that publish-codebuild.sh does not reflect changes made on CCI to split the deployment step into pieces.

This PR should either incorporate these changes OR release should be descoped from here and added later with the split.

Copy link
Member

@sobolk sobolk left a comment

Choose a reason for hiding this comment

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

Did get a chance to test it? We should at least get successful tagged release using this workflow.

Major concerns:

  • We should decouple CB release workflows from CCI sunset. These should be separate changes. Ability to use both systems should exists until we gain confidence in CB. In event of emergency we should be able to buy CCI credits and revert back to old systems should CB prevent us from shipping. Smooth migration must include period where both old and new system is accessible.
  • The necessity of codebuild-checkout in release steps 2,3,4. Revised release process assumes that outcome of build step is cached including git metadata. Can we get this fixed so that checkout is necessary only in entry steps (build, build-tests-standalone? )

Comment on lines 1 to 2
# version: 2.1
# orbs:
Copy link
Member

Choose a reason for hiding this comment

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

disabling CCI should be separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we shouldn't turn off CCI until we've done at least a couple releases using CodeBuild

Comment on lines 1 to 3
# version: 2.1

# this allows you to use CircleCI's dynamic configuration feature
setup: true
# # this allows you to use CircleCI's dynamic configuration feature
Copy link
Member

Choose a reason for hiding this comment

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

same here.

Comment on lines 3 to 4
git checkout $BRANCH_NAME

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't checkout a branch every time we load this script.
Is this intentional?

aws --profile=s3-uploader s3 cp amplify-pkg-linux-arm64.tgz s3://aws-amplify-cli-do-not-delete/$(echo $version)/amplify-pkg-linux-arm64-$(echo $hash).tgz
aws --profile=s3-uploader s3 cp amplify-pkg-linux-x64.tgz s3://aws-amplify-cli-do-not-delete/$(echo $version)/amplify-pkg-linux-x64-$(echo $hash).tgz
if [[ "$BRANCH_NAME" == "release" ]] || [[ "$BRANCH_NAME" =~ ^run-e2e-with-rc\/.* ]] || [[ "$BRANCH_NAME" =~ ^release_rc\/.* ]] || [[ "$BRANCH_NAME" =~ ^tagged-release ]]; then
aws s3 cp amplify-pkg-win-x64.tgz s3://$PKG_CLI_BUCKET_NAME/$(echo $version)/amplify-pkg-win-x64-$(echo $hash).tgz
Copy link
Member

Choose a reason for hiding this comment

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

Can we make a copy of this file instead? and drop non-CB version together with CCI later?

Please do this for all other .sh files where we made such assumption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 3 to 4
source ./.circleci/codebuild-checkout.sh

Copy link
Member

Choose a reason for hiding this comment

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

This is unexpected.
In CCI .circleci/publish-step-1-set-versions.sh adds a commit to local repo that is NOT pushed to git.
That commit is retained in other jobs because we cache result of build step, including .git directory which preserves that information. We should not need to checkout everywhere if we we cache workspace properly.

This is concern for other files that attempt to checkout after build step.

buildspec: codebuild_specs/release_workflows/github_release.yml
env:
compute-type: BUILD_GENERAL1_LARGE
debug-session: true
Copy link
Member

Choose a reason for hiding this comment

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

do we want these debug sessions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just part of it; the other part is to manually trigger the job with debug sessions enabled & ssm enabled

@codecov-commenter
Copy link

Codecov Report

Merging #12918 (c816f02) into dev (ec9a2ba) will increase coverage by 48.56%.
Report is 288 commits behind head on dev.
The diff coverage is 58.82%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff             @@
##              dev   #12918       +/-   ##
===========================================
+ Coverage    0.00%   48.56%   +48.56%     
===========================================
  Files        1296      848      -448     
  Lines      149743    38131   -111612     
  Branches     1296     7798     +6502     
===========================================
+ Hits            0    18519    +18519     
+ Misses     148447    18022   -130425     
- Partials     1296     1590      +294     
Files Changed Coverage Δ
...rmation/auth-stack-builder/auth-stack-transform.ts 91.30% <0.00%> (+91.30%) ⬆️
...tack-builder/auth-user-pool-group-stack-builder.ts 51.72% <ø> (+51.72%) ⬆️
...h-stack-builder/user-pool-group-stack-transform.ts 94.59% <0.00%> (+94.59%) ⬆️
.../src/provider-utils/awscloudformation/constants.ts 100.00% <ø> (+100.00%) ⬆️
...e-walkthrough-types/awsCognito-user-input-types.ts 100.00% <ø> (+100.00%) ⬆️
...vice-walkthrough-types/cognito-user-input-types.ts 100.00% <ø> (+100.00%) ⬆️
...y-category-function/src/commands/function/build.ts 0.00% <0.00%> (ø)
...ify-category-function/src/events/prePushHandler.ts 33.33% <0.00%> (+33.33%) ⬆️
...ider-utils/awscloudformation/utils/layerHelpers.ts 21.80% <0.00%> (+21.80%) ⬆️
...er-utils/awscloudformation/utils/storeResources.ts 30.38% <ø> (+30.38%) ⬆️
... and 52 more

... and 1249 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@sobolk sobolk left a comment

Choose a reason for hiding this comment

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

LGTM.

Comment on lines +13 to +15
artifacts:
files:
- 'shared-scripts.sh'
Copy link
Member

Choose a reason for hiding this comment

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

what is this for ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

codebuild requires a job to output an artifact, otherwise other jobs cannot depend on it downstream

Copy link
Member

Choose a reason for hiding this comment

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

:-)

version: 0.2
env:
shell: bash
git-credential-helper: yes
Copy link
Member

Choose a reason for hiding this comment

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

how doesgit-credential-helper work ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only the jobs that require .git credentials should use git-credential-helper; this injects the GH credentials via the git-credential-helper https://git-scm.com/docs/gitcredentials so that things like push work. Can only be used on jobs that are manually triggered via console/api.

.circleci/cb-publish-step-3-npm.sh Show resolved Hide resolved
# if we run into problems in the future, we should revisit this
# git reset --soft HEAD~1
git reset --soft HEAD~1
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the goal of doing a soft reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if i remember correctly; its to remove the commit that is created during publish to verdaccio step so that it doesn't affect the unified changelog step

Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't be necessary. does CCI do this ?

Copy link
Member

Choose a reason for hiding this comment

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

or maybe it does make some sense, should we unreset after step which is affected by this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, this unified changelog script is designed to work with staged files:

 // This script is intended to be run after running `yarn publish-to-verdaccio` when all CHANGELOG file changes are staged but not committed
 // It will scan all staged CHANGELOG.md files and merge them into a single UNIFIED_CHANGELOG file

@awsluja awsluja merged commit 71257ec into aws-amplify:dev Aug 3, 2023
5 checks passed
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.

5 participants