feat: support conditional AWS::NoValue in SAM Function's IAM Role

[madsid]

Issue #, if available

#3736

Description of changes

Support AWS::NoValue conditional for Role property in AWS::Serverless::Function

Role is an optional string value for AWS::Serverless::Function. An execution role is auto generated code when the value is None. This change adds support for AWS::NoValue.

Use cases :

1. Role string passed though as is.
2. Role If conditional with both AWS::NoValue creates an execution role for both true and false case and replaces AWS::NoValue
3. Role if conditional with 1 AWS::NoValue creates an execution role and replaces AWS::NoValue
4. Role value with Fn::GetAtt gets passed through as is.

Description of how you validated changes

• Added unit tests
• Hand tested sam-translate.py with templates and verified translated templates

Checklist

• Review the generative AI contribution guidelines
• Adheres to the development guidelines
• Add/update transform tests
  • Using correct values
  • Using wrong values
• Add/update integration tests

Examples?

Please reach out in the comments if you want to add an example. Examples will be
added to sam init through aws/aws-sam-cli-app-templates.

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

[valerena]

This is not exactly correct here. In theory we need to create this role only if one of the values is AWS::NoValue.

If the Role field has an If with two different roles, then we shouldn't create the new one. (so, move this resources.append to inside a new condition "if 1 or 2 are intrinsic_no_value"

[valerena]

You can do the same than for the roles with the functions: generated_functions = [x for x in cfnResources if isinstance(x, LambdaFunction)]
and then you can also confirm that there's only one created.

But it's not a big deal. We can probably change it in the future if needed.

[valerena]

You don't use the variable template anywhere else, so you could just remove it and define the conditions below. (same in other tests)

Suggested change

	template = {"Conditions": {"Condition": True}}
	kwargs = dict(self.kwargs)
	kwargs["conditions"] = template.get("Conditions", {})
	kwargs = dict(self.kwargs)
	kwargs["conditions"] = {"Condition": True}

[valerena]

You don't need this self.template variable since it's not being used. Right below you use it to get Conditions from here, but it will be just {}, so you can just do that ("conditions": {})

[roger-zhangg]

you can avoid a indentation by early return

Suggested change

	if lambda_role is None:
	resources.append(execution_role)
	lambda_function.Role = execution_role.get_runtime_attr("arn")
	elif is_intrinsic_if(lambda_role):
	if lambda_role is None:
	resources.append(execution_role)
	lambda_function.Role = execution_role.get_runtime_attr("arn")
	return
	if not is_intrinsic_if(lambda_role):
	return

[roger-zhangg]

this implementation has some unnecessary change IMO, we can keep the if lambda_function.Role is None part as is, then _make_lambda_role can return the created role, so we don't need to pass in lambda_function in this function.

[roger-zhangg]

One other concern is we are modifying values for lambda_function, execution_role, resources, conditions ( which are the function's input) inside this function which returns None, but all these values might have changed after running this function. This might make this part of the code harder to review in the future. If we need to change all these variable's value in place, I would perfer to implement this directly in to_cloudformation instead of this subfunction. If you really prefer to have this subfunction. Maybe we can return the new value instead of modifying them inplace. So future reviewers could easily understand these variables are modified in this subfunction.

[valerena]

You can change this a little bit so you return a role_changes["execution_role_resource"] that it's either execution_role or None. And you set the Condition attribute inside the function not here outside.

so then you have

if role_changes["execution_role_resource"] is not None:
    resources.append(role_changes["execution_role_resource"])

and that role comes ready to be added, you can change its resource_attribute("Condition, inside the function instead of here.

[valerena]

Another recommendation. Work with the simple variables first. And them to the result only at the very end for returning. Using the result variable for all the following manipulations makes the code look so much worse.