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: Extend BashJobRunner CodeBuild Project props. #78

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

Conversation

daveroberts-aws
Copy link

Issue # (if applicable)

Closes #.

Reason for this change

  1. There's no way to change the CodeBuild Project properties in BashJobRunner. (For example, adding a source, or changing the compute type or build image).

  2. Contributing.md didn't specify to install dependencies in the point-solutions, leading to a test failing.

Description of changes

  1. Added an optional property to BashJobRunnerProps which expands in the Project props if present.

Description of how you validated changes

  1. Added extra properties to the integration test for the core app.

Checklist

  • [ x ] My code adheres to the CONTRIBUTING GUIDE
  • [ x ] I have updated the relevant documentation (if applicable).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

@daveroberts-aws daveroberts-aws changed the title Extend BashJobRunner CodeBuild Project props. feat: Extend BashJobRunner CodeBuild Project props. Jul 29, 2024

/**
* Any additional properties for the CodeBuild project beyond the default.
* These take precedence over defaults.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea, but one concern I have is with regards to props.scriptEnvironmentVariables. With this new change, we'll have 2 parameters that can be used to set the env vars available in the codebuild project.

IMO, it would be better for us to reduce that to 1 by either:

  • removing props.scriptEnvironmentVariables
  • removing codeBuildProps and adding two new args: one for buildImage and one for computeType

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