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

feat: DBTP-1507 Setup codepipeline notifications #314

Conversation

tony-griffin
Copy link
Contributor

@tony-griffin tony-griffin commented Jan 16, 2025

Addresses DBTP-1507

Use platform-helper notify commands for notifications, similar to our other pipelines.

  • Code relied on John's codebase pipeline PR and so is branched from that.
  • Codebase module files changed in this PR:
    codebase-pipelines/buildspec-deploy.yml
    codebase-pipelines/codepipeline.tf
    codebase-pipelines/locals.tf
    codebase-pipelines/codepipeline.tf
    codebase-pipelines/tests/unit.tftest.hcl
    codebase-pipelines/iam.tf

Screenshot 2025-01-20 at 11 45 45
Screenshot 2025-01-20 at 11 25 02
Screenshot 2025-01-20 at 11 30 29


Checklist:

Title:

  • Scope included as per conventional commits
  • Ticket reference included (unless it's a quick out of ticket thing)

Description:

  • Link to ticket included (unless it's a quick out of ticket thing)
  • Includes tests (or an explanation for why it doesn't)
  • Includes any applicable changes to the documentation in this code base
  • Includes link(s) to any applicable changes to the documentation in the DBT Platform Documentation (can be to a pull request)

Tasks:

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.68%. Comparing base (a886ed1) to head (5b28002).
Report is 30 commits behind head on DBTP-1503-manual-pipeline.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@                    Coverage Diff                     @@
##           DBTP-1503-manual-pipeline     #314   +/-   ##
==========================================================
  Coverage                      81.68%   81.68%           
==========================================================
  Files                              7        7           
  Lines                            868      868           
==========================================================
  Hits                             709      709           
  Misses                           159      159           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JohnStainsby JohnStainsby changed the base branch from main to DBTP-1503-manual-pipeline January 20, 2025 09:16
@JohnStainsby JohnStainsby changed the base branch from DBTP-1503-manual-pipeline to main January 20, 2025 09:20
@tony-griffin tony-griffin marked this pull request as ready for review January 20, 2025 11:49
@tony-griffin tony-griffin requested a review from a team as a code owner January 20, 2025 11:49
tony-griffin and others added 3 commits January 20, 2025 12:36
@JohnStainsby JohnStainsby changed the base branch from main to DBTP-1503-manual-pipeline January 21, 2025 10:42
Copy link
Contributor

@antroy-madetech antroy-madetech left a comment

Choose a reason for hiding this comment

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

A few minor comments.

codebase-pipelines/buildspec-deploy.yml Outdated Show resolved Hide resolved
codebase-pipelines/buildspec-deploy.yml Outdated Show resolved Hide resolved
- aws ecr get-login-password --region ${AWS_REGION} | docker login --username AWS --password-stdin ${AWS_ACCOUNT_ID}.dkr.ecr.${AWS_REGION}.amazonaws.com

# construct Slack message env vars
- |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in general, these multiline commands should be reserved for blocks, such as if/then/else clauses, function definitions while loops etc. So I'd put line 30 in it's own single line item.

echo "Image contains no timestamp label"
exit 1
fi
echo "Found image timestamp $SLACK_REF"
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly this echo should be it's own item


# Extract the pipeline name from CODEBUILD_INITIATOR default env var
- |
PIPELINE_NAME=""
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed - the conditional will either add the PIPELINE_NAME or exit

fi

# Construct the pipeline execution URL
- |
Copy link
Contributor

Choose a reason for hiding this comment

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

Split this into 2 single line items.

echo "Pipeline execution URL: ${PIPELINE_EXECUTION_URL}"

- |
BUILD_ID_PREFIX=$(echo $CODEBUILD_BUILD_ID | cut -d':' -f1)
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

BUILD_ID_PREFIX=$(echo $CODEBUILD_BUILD_ID | cut -d':' -f1)
echo "BUILD_ID_PREFIX - ${BUILD_ID_PREFIX}"

- |
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be just a single line item. - MESSAGE=...

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 tried adding as a single line comment but there was a syntax error because of the use of the ":" which only resolved when using multiline

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI - you should be able to just surround the line in single quotes. It's because it thinks it's a map rather than a string. Surrounding in single quotes will make the YAML parser treat it as a string.

But happy for you to leave it as a multi-line quote in this case as it's probably neater.

STATUS_TEXT="STOP_REQUESTED/STOPPED/PENDING/IN_PROGRESS"
;;
esac
echo "Status emoji set to ${STATUS_EMOJI}"
Copy link
Contributor

Choose a reason for hiding this comment

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

The echo should be a command on it's own

esac
echo "Status emoji set to ${STATUS_EMOJI}"

- |
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need the block scalar indicator here as it's just a single line.

Copy link
Contributor

@antroy-madetech antroy-madetech left a comment

Choose a reason for hiding this comment

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

Looking better. A minor formatting thing left, but approved.


# Construct the pipeline execution URL
- PIPELINE_EXECUTION_URL="https://${AWS_REGION}.console.aws.amazon.com/codesuite/codepipeline/pipelines/${PIPELINE_NAME}/executions/${PIPELINE_EXECUTION_ID}"

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just remove some of these random empty lines in this section.

BUILD_ID_PREFIX=$(echo $CODEBUILD_BUILD_ID | cut -d':' -f1)
echo "BUILD_ID_PREFIX - ${BUILD_ID_PREFIX}"

- |
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI - you should be able to just surround the line in single quotes. It's because it thinks it's a map rather than a string. Surrounding in single quotes will make the YAML parser treat it as a string.

But happy for you to leave it as a multi-line quote in this case as it's probably neater.

@tony-griffin tony-griffin merged commit 5b28002 into DBTP-1503-manual-pipeline Jan 23, 2025
48 checks passed
@tony-griffin tony-griffin deleted the DBTP-1507-setup-codepipeline-notifications branch January 23, 2025 13:43
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.

4 participants