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: Skip image-repository validation of ECR images (#2934) #3015

Conversation

alexisfacques
Copy link
Contributor

Which issue(s) does this change fix?

Issue #2934

Why is this change necessary?

How does it address the issue?

  • Instead of resolving packageable Image functions from the raw template file(s) (get_template_function_resource_ids, get_template_artifacts_format), use the SamFunctionProvider to do so (evaluating template parameters values, so the template must go through the IntrinsicResolver).

What side effects does this change have?

  • get_template_function_resource_ids, get_template_artifacts_format are obsolete (not yet deleted from the repo);

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.

* fix lambda ECR image build/validation/deploy:
- change guided context to only prompt image-repository option for 
buildable, packageable lambda images;
- change CLI 'image-repository' 'image-repositories' option validation 
to only check the template for buildable, packageable lambda images.
@qingchm
Copy link
Contributor

qingchm commented Jun 17, 2022

In order for this to be properly reviewed please resolve the merge conflicts and make sure that make pr passes!

Copy link
Contributor

@qingchm qingchm left a comment

Choose a reason for hiding this comment

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

Unfortunately this does not meet the PR requirements yet, please make sure the listed requirements are met.

@alexisfacques
Copy link
Contributor Author

Heya guys! Ooops, forgot this was still WIP.
Will rebase on the current release asap

@mndeveci
Copy link
Contributor

mndeveci commented Jul 5, 2022

@alexisfacques I am marking this PR as draft for now. Please mark it ready once you addressed previous comments.

@mndeveci mndeveci marked this pull request as draft July 5, 2022 12:47
@jfuss
Copy link
Contributor

jfuss commented Aug 9, 2022

Closing as inactive.

Happy to review an updated PR from the community, if someone wants to revive the PR

@jfuss jfuss closed this Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants