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

DCMAW-11057: Push STS test image to dev and build #338

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

sandymay-dd
Copy link
Contributor

@sandymay-dd sandymay-dd commented Jan 15, 2025

What changed

Containerises the API test suite and updates the STS mock push-to-main.yml action to build/tag/publish the test image to Dev and Build.

Why did it change

This partially implements the Testing step in the Secure Pipeline methodology, whereby automated tests are run and are used to determine whether to promote the application to the next environment. The other part of the implementation will be done in the https://github.com/govuk-one-login/mobile-id-check-async-infra repository (updating the secure pipeline and test image repository Cloudformation stacks).

This is a mock asset that isn't promoted to Production; however, it is still good to run these tests so that we are alerted in case there are issues in Dev or Build that we need to address.

This also introduces a new pattern for the team - Github reusable workflows (https://docs.github.com/en/actions/sharing-automations/reusing-workflows). The intent here is to create re-usable abstracted assets helping with maintainability and improves speed of adoption for new applications. This has only been done for functionality that builds/pushes/tags the docker image used in the test containers (AWS CodeBuild)

Testing

Action successfully runs for the Dev environment. It fails for Build due to IAM policy restrictions (only the main branch can push assets to Build`; this is expected.

image

run-tests-locally.sh passes, when run against the mob-sts-mock stack

image

Checklists

  • There is a ticket raised for this PR that is present in the branch name
  • No PII data logged. See guidance here
  • Demo to a BA, TA, and the team.
  • Update README with any new instructions or tasks

- name: Setup nodeJS v20
uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af #v4.1.0
with:
node-version-file: '.nvmrc'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enforces the use of an .nvmrc file (a standard we use in all our async stacks), but I think the easiest way to ensure this is running on the version of Node that we develop with. Happy for suggestions.

jobs:
build-and-push:
name: build-and-push
runs-on: ubuntu-24.04
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this is a later version than what we use elsewhere (ubuntu-22)

version: 1.132.0

- name: Install npm packages
run: npm install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: also enforces the use of npm. This is a programme standard now and is preferred to yarn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

annoyingly - I don't think you can have sub-directories for workflows. Happy for naming suggestions to make this file stand out.

Copy link
Contributor

Choose a reason for hiding this comment

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

A cheeky prefix?

_name.yml
or
action-${name}.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or potentially job-{name}.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(ideas for tomorrow me)


build-and-upload-sam-artifact-to-dev:
name: Validate & upload S3 artifact to dev
runs-on: ubuntu-22.04
needs: sonar-scan
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe our route to live should be blocked by a Sonar main branch analysis scan. We check the quality gate has been met on our pull-request workflows.

Comment on lines +9 to +15
secrets:
GH_ACTIONS_ROLE_ARN:
required: true
TEST_IMAGE_REPOSITORY:
required: true
CONTAINER_SIGN_KMS_KEY:
required: true
Copy link
Contributor Author

@sandymay-dd sandymay-dd Jan 16, 2025

Choose a reason for hiding this comment

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

These aren't strictly secrets; however, I had issues passing these through as inputs. I would prefer to keep these as secrets in this template, to help understand provenance and maintain consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping them as secrets for the moment matches the other pipelines. I'm happy with this atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to clarify: I had technical issues, not moral ones 😀

@sandymay-dd sandymay-dd marked this pull request as ready for review January 16, 2025 16:27
@sandymay-dd sandymay-dd requested review from a team as code owners January 16, 2025 16:27
Copy link

Quality Gate Passed Quality Gate passed for 'mobile-id-check-async-sts-mock'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

COPY package.json package-lock.json /
RUN npm install --ignore-scripts

FROM node:iron-alpine AS final
Copy link
Contributor Author

@sandymay-dd sandymay-dd Jan 16, 2025

Choose a reason for hiding this comment

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

I don't think there's any value of a multi-stage build given that we're copying all the node_modules over. Will remove. Thought this was saving space at the time (oops).


stack_name=${1:-mob-sts-mock}

echo "Running test against ${stack_name}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
echo "Running test against ${stack_name}"
echo "Running tests against ${stack_name}"

# id: cosign-image
# run: |
# cosign sign --key awskms:///$BUILD_CONTAINER_SIGN_KMS_KEY $STS_MOCK_BUILD_TEST_IMAGE_REPOSITORY_URI:$IMAGE_TAG
test-image-build:
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 calls the reusable workflow job

uses:
./.github/workflows/shared-build-and-push-test-image.yml
with:
WORKING_DIRECTORY: sts-mock
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will see if there's a way to avoid needing to pass this through

docker build --tag testcontainer .

docker run --rm --interactive --tty \
--user root \
Copy link
Contributor Author

@sandymay-dd sandymay-dd Jan 16, 2025

Choose a reason for hiding this comment

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

note to me: I think this is run as the root in the pipeline and we can't change that in ID Check.

cosign-release: 'v1.9.0'

- name: Install SAM CLI
uses: aws-actions/setup-sam@819220f63fb333a9a394dd0a5cab2d8303fd17e2 #v2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed!

version: 1.132.0

- name: Install npm packages
run: npm install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could we just install --dev-dependencies

version: 1.132.0

- name: Install npm packages
run: npm install
Copy link
Contributor Author

@sandymay-dd sandymay-dd Jan 17, 2025

Choose a reason for hiding this comment

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

npm clean-install (alias for npm ci)

FROM node:iron-alpine AS builder

COPY package.json package-lock.json /
RUN npm install --ignore-scripts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm ci

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.

2 participants