This repository was archived by the owner on Oct 23, 2023. It is now read-only.
SSE-2685: Disambiguate SAM and Docker build paths #8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CharlesIC
force-pushed
the
sse-2732-ecr-in-dev
branch
from
5f42cdf to
674635e
Compare
CharlesIC
changed the title
CharlesIC
force-pushed
the
sse-2732-ecr-in-dev
branch
3 times, most recently
from
2381f69 to
0903dd2
Compare
CharlesIC
force-pushed
the
sse-2732-ecr-in-dev
branch
6 times, most recently
from
e7dfefa to
d9a1e1d
Compare
This action builds two things (SAM app and a Docker image) and therefore needs two build directories, defaulting to the current dir, as we cannot assume they're the same. We already have a docker build path so replace the "working dir" with the SAM base dir to avoid confusion. The singular "working directory" is confusing and misunderstood, which causes breaking changes to the action being made. For Docker, allow dockerfile and docker path to be different since the dockerfile doesn't necessarily always reside in the docker path so the filename cannot be prefixed with the build path by default - use the docker build args accordingly. Allow the SAM template file to be different than the base build dir - the path from which SAM resources are resolved is not always the same as the template file, which is the default, therefore the --base-dir param must be used. Don't cd into any directory as the current working dir is set by the calling workflow and it's unexpected behaviour to cd away from it. Set default working dirs in the script The "." default value doesn't get carried to the script env vars. Handle default value of null in the script itself.
CharlesIC
force-pushed
the
sse-2732-ecr-in-dev
branch
from
d9a1e1d to
0645481
Compare
CharlesIC
changed the title
This action builds two things (SAM app and a Docker image) and therefore needs two build directories, defaulting to the current dir, as we cannot assume they're the same. We already have a docker build path so replace the "working dir" with the SAM base dir to avoid confusion. The singular "working directory" is confusing and misunderstood, which causes breaking changes to the action being made. The working-directory input is retained for backwards compatibility and is used as the SAM build dir if the new param is not set. For Docker, allow dockerfile and docker path to be different since the dockerfile doesn't necessarily always reside in the docker path so the filename cannot be prefixed with the build path by default - use the docker build args accordingly. Allow the SAM template file to be different than the base build dir - the path from which SAM resources are resolved is not always the same as the template file, which is the default, therefore the --base-dir param must be used. Don't cd into any directory as the current working dir is set by the calling workflow and it's unexpected behaviour to cd away from it. Set default working dirs in the script The "." default value doesn't get carried to the script env vars. Handle default value of null in the script itself.
| template-file: | ||
| description: "The name of the CF template for the application. This defaults to template.yaml" | ||
| required: false | ||
| default: template.yaml | ||
| working-directory: | ||
| description: "The working directory containing the app" | ||
| description: "Deprecated - use sam-base-directory instead" |
There was a problem hiding this comment.
is this a breaking change and will we need a new major release? Can you update the readme also :)
# for free
to subscribe to this conversation on GitHub.
Already have an account?
#.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Disambiguate SAM and Docker build paths
This action builds two things (SAM app and a Docker image) and therefore needs two build directories, defaulting to the current dir, as we cannot assume they're the same.
We already have a docker build path so replace the "working dir" with the SAM base dir to avoid confusion. The singular "working directory" is confusing and misunderstood, which causes breaking changes to the action being made.
For Docker, allow dockerfile and docker path to be different since the dockerfile doesn't necessarily always reside in the docker path so the filename cannot be prefixed with the build path by default - use the docker build args accordingly.
Allow the SAM template file to be different than the base build dir - the path from which SAM resources are resolved is not always the same as the template file, which is the default, therefore the --base-dir param must be used.
Don't cd into any directory as the current working dir is set by the calling workflow and it's unexpected behaviour to cd away from it.
Set default working dirs in the script as the "." default value doesn't get carried to the script env vars. Handle default value of null in the script itself.
Ticket number
SSE-2685
Checklist
I have updated the changelog
I have tested this and added output to Jira
Comment:
A workflow run where this version of the action is used - the action succeeds (the overall workflow fails due to an unrelated issue)
https://github.com/alphagov/di-onboarding-self-service-experience/actions/runs/5951180253/job/16140513640#step:2:573
Comment: