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

Add AMI build job to build workflow #77

Merged
merged 9 commits into from
Jul 22, 2021

Conversation

jsf9k
Copy link
Member

@jsf9k jsf9k commented Jul 9, 2021

🗣 Description

This pull request adds a build task to the build workflow. This new task goes through the process of building a new AMI but does not actually register it.

This pull request is blocked until we move to Packer 1.7 (see #70 and cisagov/setup-env-github-action#32), since that is where support for the skip_create_ami key was added for Packer configurations (see the first bullet).

💭 Motivation and context

This is a useful feature to add since it will catch changes that cause the AMI build process to fail. Currently these changes are only caught when we create a pre-release or release.

N.B.: Once we have dev, staging, and production COOL environments we could allow this job to go ahead and register the AMI in the dev environment. That would allow us to fully test in the dev environment before we create a pre-release (for staging) and/or a release (for production).

🧪 Testing

cisagov/setup-env-github-action#32 was merged and now all pre-commit hooks and GitHub Actions workflows pass.

✅ Checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.
  • Add build/build job to the list of GH Actions jobs that must pass before a PR can be merged.

jsf9k added 2 commits July 9, 2021 10:53
The test build goes through the process of building an AMI but does
not actually create it.  This is an good thing to add since it will
catch changes that cause the AMI build process to fail.  Currently
these changes are only caught when we create a pre-release or release.
The build job can be an expensive test, so we don't want to bother
running it unless the other jobs pass.
@jsf9k jsf9k added blocked This issue or pull request is awaiting the outcome of another issue or pull request improvement This issue or pull request will add or improve functionality, maintainability, or ease of use labels Jul 9, 2021
@jsf9k jsf9k self-assigned this Jul 9, 2021
@jsf9k jsf9k force-pushed the improvement/add-ami-build-to-build-workflow branch from 1df0ec7 to f899682 Compare July 9, 2021 14:56
@jsf9k jsf9k changed the title Add AMI build to build workflow Add AMI build job to build workflow Jul 9, 2021
@jsf9k jsf9k marked this pull request as ready for review July 11, 2021 14:41
@jsf9k jsf9k requested review from dav3r, felddy and mcdonnnj as code owners July 11, 2021 14:41
Copy link
Member

@felddy felddy left a comment

Choose a reason for hiding this comment

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

This is a good improvement. It should catch a whole new class of errors.
Strong work. 💪

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Excellent! 🎸
I only noted one small thang.

.github/workflows/build.yml Outdated Show resolved Hide resolved
Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

This is a great improvement. I have some suggestions that are mostly to bring this into alignment with the work in cisagov/skeleton-generic#82.

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@jsf9k jsf9k requested a review from mcdonnnj July 13, 2021 20:24
Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

just some last suggestions. Thanks again for thinking of and implementing this improvement!

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@mcdonnnj mcdonnnj merged commit e32adc7 into develop Jul 22, 2021
@mcdonnnj mcdonnnj deleted the improvement/add-ami-build-to-build-workflow branch July 22, 2021 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue or pull request is awaiting the outcome of another issue or pull request improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
Development

Successfully merging this pull request may close these issues.

4 participants