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

OC-926: Make ECS task run incremental ARI import #747

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

Conversation

finlay-jisc
Copy link
Collaborator

@finlay-jisc finlay-jisc commented Jan 6, 2025

The purpose of this PR was to alter the existing proof-of-concept ECS task to run the ARI import process. This involved:

  • Changing the naming of various things in the terraform code to reflect the purpose of the ECS task (from "hello world").
  • Add a compose file and command script so that the task container can be run locally.
  • Allow the report format of the ARI import job to be set via an arg when the script is run.

Tested on int environment by manually changing an ARI publication's title, then running the task via its API endpoint and confirming that the script ran, updated the title back to how it should be, and reported the results appropriately.


Acceptance Criteria:

The ECS task runs an incremental ARI import successfully.


Checklist:

  • Local manual testing conducted
  • Automated tests added
  • Documentation updated

Tests:

E2E
Screenshot 2025-01-06 125102

API
Screenshot 2025-01-06 130401


Screenshots:

UpdatedAt field is changed in the DB at the point the script was run and corrected the publication title.
Screenshot 2025-01-06 112257

Canonical DOI of publication is updated by the script after updating the publication title.
Screenshot 2025-01-06 112502

Publication has been reindexed in opensearch, and appears at the top because it has the most recent updatedAt value.
Screenshot 2025-01-06 112702

Task log
Screenshot 2025-01-06 112733

Output email sent to slack channel
Screenshot 2025-01-06 112823

return {
importAllDepartments: !!allDepartmentsArg,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These were actually always setting the property to true, so I changed the way they're set.

build:
context: .
command:
["/bin/sh", "-c", "/app/infra/docker/ariImportRunner/local-ari-import.sh"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Override the container's command locally - only difference is to change the report format to a file because the container can't send an email.

@@ -19,8 +19,8 @@ resource "aws_codebuild_project" "deploy-docker-image" {

environment {
compute_type = "BUILD_GENERAL1_SMALL"
image = "aws/codebuild/standard:5.0"
type = "LINUX_CONTAINER"
image = "aws/codebuild/amazonlinux-aarch64-standard:3.0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The codepipeline to build the image wasn't working until I changed this to build with an ARM based environment.

@finlay-jisc finlay-jisc marked this pull request as ready for review January 6, 2025 13:04
@finlay-jisc finlay-jisc requested a review from a team as a code owner January 6, 2025 13:04
openssl \
openssl-dev \
libc6-compat
RUN git clone https://github.com/JiscSD/octopus /app

Choose a reason for hiding this comment

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

How come you're cloning the repo in the Dockerfile? This is already in the repo, so things could just be copied in

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's true, I'll try it with a copy

@@ -1,3 +1,4 @@
# syntax=docker/dockerfile:1.7-labs

Choose a reason for hiding this comment

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

Haven't seen this before, what does it do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was needed to use the --exclude flag with COPY.

Copy link

@adwearing-jisc adwearing-jisc Jan 9, 2025

Choose a reason for hiding this comment

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

# syntax=docker/dockerfile:1.7-labs
FROM public.ecr.aws/docker/library/node:20-alpine

RUN apk add \
    ca-certificates \
    curl \
    gnupg \
    git \
    openssl \
    openssl-dev \
    libc6-compat

WORKDIR /app

COPY package.json .
RUN npm i
COPY . .
CMD ["npm", "run", "ariImport", "--", "dryRun=false", "reportFormat=email"]

Alternative way of doing it, removes the mkdir command, WORKDIR does this for you. Splits things into multiple steps so you don't have to redo npm i everytime a file changes. Also seems to be better to set context root to the api folder, and only copy over the api stuff rather than everything (the above example assumes this change).

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