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

fix: Revert the behavior when both ImageUri and Metadata exist for im… #3272

Merged
merged 2 commits into from
Sep 14, 2021

Conversation

aahung
Copy link
Contributor

@aahung aahung commented Sep 14, 2021

…age func

Which issue(s) does this change fix?

Why is this change necessary?

Fix #3239 by breaking change #2935
When both ImageUri and Metadata are provided, previously we choose to give "Metadata" the priority and build it. But now we skip it because ImageUri is provided.

How does it address the issue?

Fix the logics so that function is only skipped when ImageUri is provided and related fields cannot be found in Metadata. Previous behavior is partially recovered.

What side effects does this change have?

N/A

Checklist

  • Add input/output type hints to new functions/methods
  • Write design document (Do I need to write a design document?)
  • Write unit tests
  • Write/update functional tests
  • Write/update integration tests
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aahung aahung requested a review from awood45 September 14, 2021 00:16
@aahung aahung requested a review from hoffa September 14, 2021 20:15
Copy link
Member

@awood45 awood45 left a comment

Choose a reason for hiding this comment

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

This looks well-formed, let's make sure another lead agrees that this is the behavior we want going forward.

@aahung aahung requested a review from mndeveci September 14, 2021 20:19
Copy link
Contributor

@mndeveci mndeveci left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this issue 👏🏼

@sriram-mv sriram-mv self-requested a review September 14, 2021 21:24
@aahung aahung merged commit 8496cbd into aws:develop Sep 14, 2021
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.

sam build broken after upgrading SAM CLI when using ECR (backward compatibility issue)
5 participants