Simplify Release Process #1677

jmgrady · 2022-06-10T18:49:45Z

This PR updates the release process for The Combine. The changes involve:

The build process creates a release string from information from git, specifically, git describe, and stores it in a file, release.js.
The release.js file is built into the container image for The Combine's frontend component.
The release information in release.js is managed by the RuntimeConfig object; the UI now fetches the release string from this object instead of the UI package version.
docs/release_process/README.md is updated with instructions for the simplified release process.

In addition, the GitHub Actions for CI/CD are changed as follows:

The docker test builds are only run when a pull request is updated; they are no longer run when a change is pushed to master since the components will all be built as part of installing the updated code on the QA server.
The live server is updated when a release is published and is built off of the software that has been tagged. This obviates the need for the live branch.
The Combine or its components are built using the project build script instead of the provided action, docker/build-push-action. The provided action works off of the GitHub reference and not on a checked out copy so the release.js cannot be built for the images.

This change is

# Conflicts: # package.json

lgtm-com · 2022-06-10T18:55:42Z

This pull request introduces 2 alerts when merging 887ecd3 into 9850e84 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

imnasnainaec

Reviewed 11 of 23 files at r1.
Reviewable status: 11 of 30 files reviewed, 3 unresolved discussions (waiting on @jmgrady)

package.json line 3 at r2 (raw file):

{
  "name": "thecombine",
  "version": "1.0.0",

Combine v1--this calls for a celebration!

...though I see this has no bearing on the behavior of npm run set-release in the branch. Is this in anticipation of a release once this branch is merged into master?

docs/release_process/README.md line 16 at r2 (raw file):

- the user has write permissions for _The Combine_ project on _GitHub_.

## Using the GitHub Interface to New Release

I feel like a verb is needed before "New Release"

Code quote:

## Using the GitHub Interface to New Release

src/types/runtimeConfig.ts line 63 at r2 (raw file):

      return window.release;
    }
    return "0.0.0-default.0";

v-initial to match style?

jmgrady

Reviewable status: 11 of 30 files reviewed, 1 unresolved discussion (waiting on @jmgrady)

docs/release_process/README.md line 16 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

I feel like a verb is needed before "New Release"

Done

src/types/runtimeConfig.ts line 63 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

v-initial to match style?

Done

codecov-commenter · 2022-06-14T16:07:04Z

Codecov Report

Merging #1677 (5804383) into master (9850e84) will increase coverage by 28.12%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1677       +/-   ##
===========================================
+ Coverage   52.16%   80.28%   +28.12%     
===========================================
  Files         272       37      -235     
  Lines        8433     3490     -4943     
  Branches      615        0      -615     
===========================================
- Hits         4399     2802     -1597     
+ Misses       3540      688     -2852     
+ Partials      494        0      -494

Flag	Coverage Δ
backend	`80.28% <ø> (+0.05%)`	⬆️
frontend	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/components/AppBar/UserMenu.tsx
src/components/LandingPage/BottomBar.tsx
src/types/runtimeConfig.ts
src/goals/ValidateChars/ValidateChars.ts
src/components/DataEntry/index.ts
...rc/components/ProjectScreen/ChooseProject/index.ts
...pGoal/MergeDupComponent/MergeDupContinueDialog.tsx
src/resources/dictionaries/en-us.dic.ts
src/components/Pronunciations/Recorder.ts
src/components/Login/SignUpPage/index.ts
... and 226 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9850e84...5804383. Read the comment docs.

jmgrady

Reviewable status: 11 of 30 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec and @jmgrady)

package.json line 3 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Combine v1--this calls for a celebration!

...though I see this has no bearing on the behavior of npm run set-release in the branch. Is this in anticipation of a release once this branch is merged into master?

A new release is not expected with the completion of this merge. The change in the version here is intended to mark the split from using the UI version as the release name. Once this is merged into master, the release name will come from the GitHub tag for the software.

imnasnainaec

Reviewed 2 of 23 files at r1, 1 of 2 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: 15 of 30 files reviewed, all discussions resolved (waiting on @jmgrady)

jasonleenaylor

Reviewed 2 of 23 files at r1, 6 of 8 files at r2, 1 of 2 files at r4.
Reviewable status: 24 of 30 files reviewed, all discussions resolved (waiting on @jmgrady)

.github/actions/combine-build/action.yml line 46 at r4 (raw file):

    - name: Build The Combine
      run: |
        deploy/scripts/build.py --tag ${{ env.IMAGE_TAG }} --repo ${{ inputs.image_repo }}

I'm sure this is set somewhere in this PR...

Code quote:

env.IMAGE_TAG

.github/workflows/backend.yml line 69 at r4 (raw file):

  docker_build:
    runs-on: ubuntu-22.04

Did we have problems on latest?

Code quote:

ubuntu-22.04

jmgrady

Reviewable status: 24 of 30 files reviewed, all discussions resolved (waiting on @jmgrady)

.github/actions/combine-build/action.yml line 46 at r4 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

I'm sure this is set somewhere in this PR...

Lines 23-26:

    - name: Create Image Tag
      run: |
        echo "IMAGE_TAG="`deploy/scripts/app_release.py --get` >> $GITHUB_ENV
      shell: bash

.github/workflows/backend.yml line 69 at r4 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

Did we have problems on latest?

ubuntu-latest is 20.04 which uses Python 3.8; our current python tests only support 3.9 & 3.10. That was the only reason for the switch. One feature of the latter flavors of python is that the return value of subprocess.run is templated structure (either string or byte array) so the type checking is better. It would probably be better to use ubuntu-latest and restore 3.8 to the Python test matrix.

imnasnainaec

Reviewable status: 17 of 31 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec, @jasonleenaylor, and @jmgrady)

.github/workflows/python.yml line 14 at r5 (raw file):

    strategy:
      matrix:
        python-version: ["3.10", "3.9", "3.10"]

I think you meant to add back "3.8".

jmgrady

Reviewable status: 17 of 31 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec, @jasonleenaylor, and @jmgrady)

.github/workflows/python.yml line 14 at r5 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

I think you meant to add back "3.8".

Yes, I did. Thanks for catching that.

- push releases to public AWS ECR repo (Note: /q8u7w4z6 will be replaced with /thecombine if the alias is approved - build frontend/backend/maintenance in parallel - pin node to 16.14 for lint_test_build

Update to use later version of aws cli tools.

imnasnainaec

Reviewed 1 of 8 files at r2, 1 of 2 files at r3, 2 of 11 files at r5, 7 of 11 files at r6, all commit messages.
Reviewable status: 27 of 35 files reviewed, 2 unresolved discussions (waiting on @jasonleenaylor and @jmgrady)

deploy/scripts/setup_cluster.py line 4 at r6 (raw file):

"""Install the pre-requisite helm charts for the Combine on a k8s cluster."""

from __future__ import annotations

Which line(s] require[s) this import?

.github/workflows/deploy_release.yml line 26 at r6 (raw file):

          aws_access_key_id: ${{ secrets.AWS_ACCESS_KEY_ID }}
          aws_secret_access_key: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
          aws_default_region: us-east-1

Why not use secrets.AWS_DEFAULT_REGION here anymore?

jmgrady

Reviewable status: 27 of 35 files reviewed, 2 unresolved discussions (waiting on @imnasnainaec, @jasonleenaylor, and @jmgrady)

deploy/scripts/setup_cluster.py line 4 at r6 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Which line(s] require[s) this import?

This is required for the script to run under Python 3.8. With Python 3.9, the CompletedProcess class is now a templated class, such as CompletedProcess[str] which specifies that the stdout and stderr fields (and others) are string types instead of a byte array.

.github/workflows/deploy_release.yml line 26 at r6 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Why not use secrets.AWS_DEFAULT_REGION here anymore?

Because the region for the public registries is always us-east-1 regardless of the account's default region. See the section To authenticate Docker to an Amazon ECR registry with get-login-password in https://docs.aws.amazon.com/AmazonECR/latest/public/public-registries.html

jmgrady

Reviewable status: 27 of 35 files reviewed, 2 unresolved discussions (waiting on @imnasnainaec, @jasonleenaylor, and @jmgrady)

.github/workflows/deploy_release.yml line 26 at r6 (raw file):

Previously, jmgrady (Jim Grady) wrote…

Because the region for the public registries is always us-east-1 regardless of the account's default region. See the section To authenticate Docker to an Amazon ECR registry with get-login-password in https://docs.aws.amazon.com/AmazonECR/latest/public/public-registries.html

I will add a comment to this effect in the YAML file.

- Add comment to explain why region is always us-east-1 - Correct registry alias

imnasnainaec

Reviewed 1 of 23 files at r1, 2 of 11 files at r5, 1 of 1 files at r7, all commit messages.
Reviewable status: 31 of 35 files reviewed, 2 unresolved discussions (waiting on @jasonleenaylor and @jmgrady)

.github/actions/combine-build/action.yml line 5 at r7 (raw file):

inputs:
  image_registry:
    description: "Docker Image Repository"

Is "Repository" still an apt descriptor?

.github/workflows/deploy_qa.yml line 32 at r7 (raw file):

    env:
      RM_PATTERN_1: \d+\.\d+\.\d+-master\.\d+
      RM_PATTERN_2: \d+\.\d+\.\d+-[a-z]+\.\d+-master\.\d+

Do these not require \ as was in QA_IMAGE_PATTERN?

Code quote:

      RM_PATTERN_1: \d+\.\d+\.\d+-master\.\d+
      RM_PATTERN_2: \d+\.\d+\.\d+-[a-z]+\.\d+-master\.\d+

jmgrady

Reviewable status: 31 of 35 files reviewed, 2 unresolved discussions (waiting on @imnasnainaec, @jasonleenaylor, and @jmgrady)

.github/actions/combine-build/action.yml line 5 at r7 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Is "Repository" still an apt descriptor?

No it is not. Corrected.

.github/workflows/deploy_qa.yml line 32 at r7 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Do these not require \ as was in QA_IMAGE_PATTERN?

No they don't because now they are quoted on the command line. I felt this makes the regex easier to read.
Before:

--remove ${{ env.QA_IMAGE_PATTERN }}

After:

--remove "${{ env.RM_PATTERN_1 }}" "${{ env.RM_PATTERN_2 }}"

imnasnainaec

Reviewed 2 of 11 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: 34 of 35 files reviewed, 1 unresolved discussion (waiting on @jmgrady)

deploy/scripts/build.py line 91 at r8 (raw file):

        log_level = logging.INFO
    else:
        log_level = logging.WARNING

How about a 1-liner:
log_level = logging.INFO if args.output_level != "none" else logging.WARNING

Code quote:

    if args.output_level != "none":
        log_level = logging.INFO
    else:
        log_level = logging.WARNING

jmgrady

Reviewable status: 34 of 35 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec and @jmgrady)

deploy/scripts/build.py line 91 at r8 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

How about a 1-liner:
log_level = logging.INFO if args.output_level != "none" else logging.WARNING

Although it is one line, I find that structure harder to read and mentally parse. I prefer to leave it as it is.

imnasnainaec

Reviewable status: 34 of 35 files reviewed, 3 unresolved discussions (waiting on @jmgrady)

deploy/scripts/app_release.py line 21 at r8 (raw file):

    match = re.search(r"^v?(\d+\.\d+\.\d+)(-\S+\.\d+)?$", result.stdout)
    if match:
        return match.expand(r"v\1\2")

Since the second (...) is ?'d, will this return have an issue if match.lastindex = 1?

Code quote:

    match = re.search(r"^v?(\d+\.\d+\.\d+)(-\S+\.\d+)?$", result.stdout)
    if match:
        return match.expand(r"v\1\2")

deploy/scripts/app_release.py line 75 at r8 (raw file):

    Allow calling from the command line.

    Allow calling from the command line so that node scripts can get/set the release.

Redundant?

Code quote:

    """
    Allow calling from the command line.

    Allow calling from the command line so that node scripts can get/set the release.
    """

deploy/scripts/app_release.py line 83 at r8 (raw file):

        set_release(get_release(), Path(args.path))
    elif args.set is not None:
        set_release(args.set, Path(args.path))

I take it you've no interest in doing both a --get printout and a --set-current simultaneously.

Code quote:

    if args.get:
        print(get_release())
    elif args.set_current:
        set_release(get_release(), Path(args.path))
    elif args.set is not None:
        set_release(args.set, Path(args.path))

jmgrady

Reviewable status: 34 of 35 files reviewed, 3 unresolved discussions (waiting on @imnasnainaec and @jmgrady)

deploy/scripts/app_release.py line 21 at r8 (raw file):
It should be fine. From the Python documentation for re:

Match.expand(template)
Return the string obtained by doing backslash substitution on the template string template, as done by the sub() method. Escapes such as \n are converted to the appropriate characters, and numeric backreferences (\1, \2) and named backreferences (\g<1>, \g) are replaced by the contents of the corresponding group.

Changed in version 3.5: Unmatched groups are replaced with an empty string.

deploy/scripts/app_release.py line 75 at r8 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Redundant?

Corrected docstring.

deploy/scripts/app_release.py line 83 at r8 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

I take it you've no interest in doing both a --get printout and a --set-current simultaneously.

Do you think it is useful? So far, the only use for the command line is --get in the GitHub Actions and --set-current in the node.js script when it is called from npm start and others. The build script for the docker containers uses the git calls and not the current contents of the release.js, so I don't know how useful that would be. In fact, perhaps the --set option should be removed.

I have pushed a change that makes the options mutually exclusive so that the help prints out:

usage: app_release.py [-h] [--get | --set SET | --set-current] [--path PATH]

Let me know if you would prefer that I back the change out.

imnasnainaec

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jmgrady)

* Get Release version from git history rather than package.json * Update release process documentation * Update Rancher version for install * User build.py in github action instead of docker/build-push-action * update combine version in package-lock.json * Remove printVersion.js - superceded by app_release.py * Update CI/CD actions * Deploy release build to QA server as well as Live server * Pin frontend_builder to 'node:16.14' * import future annotations for Python 3.8 * Replace os.system call with subprocess.run in build.py - push releases to public AWS ECR repo - build frontend/backend/maintenance in parallel - pin node to 16.14 for lint_test_build * Update version of sillsdev/aws-kubectl

Fix error introduced in PR #1677. Previous implementation used os.system which takes a string. #1677 changed to use subprocess.run which takes a List. When building for Rancher Desktop, the namespace was part of the build_cmd string instead of separate elements in the list.

jmgrady added 10 commits June 6, 2022 09:42

Get Release version from git history rather than package.json

ff645af

Update release process documentation

7e368fc

Update Rancher version for install

ecd365f

Finish update to store release name in release.js

de6ea88

Add --set-current option to app_release.py

97ea1c6

User build.py in github action instead of docker/build-push-action

a3b4378

Use more concise version report for kubectl

9e3e77e

Merge branch 'master' into refactor-version

2c257fd

# Conflicts: # package.json

update combine version in package-lock.json

14c6574

Remove printVersion.js - superceded by app_release.py

887ecd3

jmgrady added documentation Improvements or additions to documentation deployment python versioning CI/CD labels Jun 10, 2022

jmgrady requested a review from imnasnainaec June 10, 2022 18:49

jmgrady self-assigned this Jun 10, 2022

jmgrady added 4 commits June 10, 2022 14:59

Update CI/CD actions

c70ddba

Remove dead code

1e8ae5f

Add --legacy-peer-deps to npm commands

57e1626

Revert to ubuntu-latest (20.04) for actions that do not need Python 3.10

901b0bf

jmgrady changed the title ~~Refactor version~~ Simplify Release Process Jun 10, 2022

imnasnainaec reviewed Jun 14, 2022

View reviewed changes

Corrections from review

7aefda0

jmgrady commented Jun 14, 2022

View reviewed changes

Deploy release build to QA server as well as Live server

03e6de8

jmgrady commented Jun 14, 2022

View reviewed changes

Pin frontend_builder to 'node:16.14'

dcdcf09

imnasnainaec reviewed Jun 17, 2022

View reviewed changes

jasonleenaylor reviewed Jun 17, 2022

View reviewed changes

jmgrady commented Jun 17, 2022

View reviewed changes

jmgrady added 2 commits June 17, 2022 11:35

Restore ubuntu-latest on GitHub Actions; add Python 3.8 to test matrix

9535c77

import future annotations for Python 3.8

cc5cc98

imnasnainaec reviewed Jun 17, 2022

View reviewed changes

Fix test matrix for Python

b17f690

jmgrady commented Jun 17, 2022

View reviewed changes

jmgrady added 3 commits June 18, 2022 06:08

Replace os.system call with subprocess.run in build.py

6898804

Refactor workflows

fffed80

- push releases to public AWS ECR repo (Note: /q8u7w4z6 will be replaced with /thecombine if the alias is approved - build frontend/backend/maintenance in parallel - pin node to 16.14 for lint_test_build

Update version of sillsdev/aws-kubectl

5804383

Update to use later version of aws cli tools.

imnasnainaec reviewed Jun 21, 2022

View reviewed changes

jmgrady commented Jun 21, 2022

View reviewed changes

Update action to deploy release

6d74791

- Add comment to explain why region is always us-east-1 - Correct registry alias

imnasnainaec reviewed Jun 22, 2022

View reviewed changes

Update comment

b0f9b8c

jmgrady commented Jun 22, 2022

View reviewed changes

imnasnainaec reviewed Jun 23, 2022

View reviewed changes

jmgrady commented Jun 23, 2022

View reviewed changes

imnasnainaec reviewed Jun 23, 2022

View reviewed changes

Fix docstring; make app_release.py args mutually exclusive

7d22ee1

jmgrady commented Jun 23, 2022

View reviewed changes

imnasnainaec approved these changes Jun 23, 2022

View reviewed changes

jmgrady merged commit a9bfe6a into master Jun 23, 2022

jmgrady deleted the refactor-version branch June 23, 2022 21:08

jmgrady mentioned this pull request Jul 1, 2022

Update image_repo references #1680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify Release Process #1677

Simplify Release Process #1677

jmgrady commented Jun 10, 2022 •

edited by jasonleenaylor

Loading

lgtm-com bot commented Jun 10, 2022

imnasnainaec left a comment

jmgrady left a comment

codecov-commenter commented Jun 14, 2022 •

edited

Loading

jmgrady left a comment

imnasnainaec left a comment

jasonleenaylor left a comment

jmgrady left a comment

imnasnainaec left a comment

jmgrady left a comment

imnasnainaec left a comment

jmgrady left a comment

jmgrady left a comment

imnasnainaec left a comment

jmgrady left a comment

imnasnainaec left a comment

jmgrady left a comment

imnasnainaec left a comment

jmgrady left a comment

imnasnainaec left a comment

Simplify Release Process #1677

Simplify Release Process #1677

Conversation

jmgrady commented Jun 10, 2022 • edited by jasonleenaylor Loading

lgtm-com bot commented Jun 10, 2022

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 14, 2022 • edited Loading

Codecov Report

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jasonleenaylor left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady commented Jun 10, 2022 •

edited by jasonleenaylor

Loading

codecov-commenter commented Jun 14, 2022 •

edited

Loading