fix: wrongful IMAGE function options validation (aws#2934)

[alexisfacques]

Which issue(s) does this change fix?

Issue #2934

Why is this change necessary?

• sam deploy fails with error Error: Missing option '--image-repositories', '--image-repository', '--resolve-image-repos', even when only valid ECRs IMAGE Lambdas are configured;
• Finally resolves issues from Skip Docker Lambda image builds when ImageUri is a valid ECR location #2934 , introduced in PR fix: Skip build of Docker image if ImageUri is a valid ECR URL (#2934) #2935;

How does it address the issue?

• Reuses the newly introduced _is_all_image_funcs_provided function, which validates that any buildable IMAGE Lambda has a valid ECR location provided; instead of only checking for any IMAGE lambdas in the template.
• Error prompt should only show if: no --image-repositories option is set and no '--image-repository' or no '--resolve-image-repos' is set if the _is_all_image_funcs_provided function returns a falsy result

What side effects does this change have?

• Extensive refactoring to samconfig and cli_validation unit tests;
• Change in CLI options validation for many commands including package, deploy, sync (Both ZIP and Image lambdas);

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• Add input/output type hints to new functions/methods
• Write design document if needed (Do I need to write a design document?)
• Write/update unit tests
• Write/update integration tests
• Write/update functional tests if needed
• 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.

[lucashuy]

Thanks for following up on this issue after all this time! Left a comment about testing

[alexisfacques]

Hi there @lucashuy;
While implementing unit tests, turns out there was a little more to this story.

The required flag seems somewhat redundant now with the introduction of _is_all_image_funcs_provided. This is because if not all image functions are provided then options become required.
I've proposed a refactoring that eliminates the use of this flag, which in turns, removes a module dependency, leading to a more extensive set of changes.

I've made adjustments to the unit tests, specifically to the cli_validation, to encompass various scenarios:

• Given that all Image functions are provided (whether they consist of only ZIP lambdas functions, or ECR Image lambdas), no option should be required.
• If --image-repositories is set but not all image functions are provided, an error indicating incomplete repositories should always be prompted to favour to the missing options one.
• If not all image functions are provided & --image-repository is set, no error should occur.
• If not all image functions are provided & --image-repository is unset, then only should a missing option error occur.

These changes could potentially impact the CLI validation behavior across multiple sam commands, notably including package, deploy, and sync; so lets make sure we don't forget anything if we move forward.

[lucashuy]

Thanks for that catch! That is a good call out regarding the other commands that also use this decorator/validation. Since we are still checking if all the Lambdas that are image based, have ECR URLs, I don't imagine that it would be an issue to update with the new method call, however will keep an eye out on the tests.

[moelasmar]

why not to check all parameters in one condition like and not (image_repositories or image_repository or resolve_image_repos) instead of having them in 2 conditions?

[moelasmar]

could you please update the existing integration test cases to add a new test cases that cover this issue (a case that all lambda functions have an ECR uri defined in the template, and another case to cover the case if some of the functions has an ECR uri, and some other functions will be provided by sam deploy parameters

[moelasmar]

Thanks @alexisfacques for your contribution, please let us know if you need any help to update the code or add integration test cases.

[mndeveci]

Closing this PR due to staleness. Please re-open it once you will be able to address the previous comments. Thanks!