fix(cli): use image digest instead of commit id when deploying #2196
Merged
mergify[bot] merged 5 commits intoaws:mainlinefrom Apr 26, 2021
Merged
fix(cli): use image digest instead of commit id when deploying #2196mergify[bot] merged 5 commits intoaws:mainlinefrom
mergify[bot] merged 5 commits intoaws:mainlinefrom
Conversation
efekarakus
changed the title
efekarakus
force-pushed
the
deploy-no-tag
branch
from
706aff0 to
17bd2bc
Compare
Lou1415926
reviewed
Apr 22, 2021
|
|
||
| // GetLocation returns the location of the ECR image. | ||
| // GetLocation returns the ECR image URI. | ||
| // If a tag is provided for the image than prioritize refering to the image via the tag. |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| // If a tag is provided for the image than prioritize refering to the image via the tag. | |
| // If a tag is provided for the image then prioritize refering to the image via the tag. |
| return err | ||
| } | ||
| o.imageTag = tag | ||
| o.imageTag = imageTagFromGit(o.cmd, o.imageTag) |
Contributor
There was a problem hiding this comment.
Should this be moved to, for example, Execute() since we are no longer asking for it?
Contributor
Author
There was a problem hiding this comment.
That makes sense! I've done this, I'll send a follow-up PR to move git.go inside the exec pkg. So that we have one package for running any commands, and increase our unit test coverage by moving it outside of the cli pkg.
| wantedErrorS string | ||
| }{ | ||
| "prompt for all options": { | ||
| "sets the image tag if not in a git repository": { |
Contributor
There was a problem hiding this comment.
Suggested change
| "sets the image tag if not in a git repository": { | |
| "leaves the image tag empty if not in a git repository": { |
iamhopaul123
approved these changes
Apr 22, 2021
Contributor
iamhopaul123
left a comment
iamhopaul123
left a comment
There was a problem hiding this comment.
+1 to wanxian's feedback otherwise looks good to me!
| // GetLocation returns the ECR image URI. | ||
| // If a tag is provided for the image than prioritize refering to the image via the tag. | ||
| // If a digest is present then refer to the image via the digest. | ||
| // Otherwise, use the "latest" tag. |
Contributor
There was a problem hiding this comment.
Maybe be more specific about it: currently the only use case is when doing package command with the default tag.
thrau
pushed a commit
to localstack/copilot-cli-local
that referenced
this pull request
Dec 9, 2022
) Fixes aws#2139 _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Fixes #2139
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.