Can you illustrate the optional properties of S3Action in this example? Especially how to use the substitution templates.

Oh. It is important for S3Action! I will illustrate it!

Will every Action have a role property? Because if the answer is "yes", we should introduce a common interface that all Action prop interfaces extend. Like the CommonActionProps in @aws-cdk/aws-codepipeline.

@skinny85
Not every Action is need a role, but almost. 2 actions in all actions will not have a role property.

• HTTP Action
• Lambda Action

For example, LambdaFunctionAction constructor (most simple one) is as following:

constructor(private readonly func: lambda.IFunction) {}

So I thought there are no common property for all actions. But the way to align the arguments of the action's constructor is what I was also struggling with.

What do you think? I'm open to any and all refactoring!

Ok, so if all of actions besides two will need role, I think introducing a separate common interface makes sense. And make S3ActionProps (soon to be changed to S3PutObjectActionProps I hope 😉) extend this new interface.

OK! I will! 🚀

@skinny85
If we create CommonActionProps and CommonAwsActionProps, should these constructors be unified in the following form?

interface CommonActionProps {
}

intercace CommonAwsActionProps extends CommonActionProps {
  role?: iam.IRole;
}

class Action {
  constructor(props: CommonActionProps) {}
}

Now, three Actions LambdaFunctionAction, CloudWatchLogsAction and S3PutObjectActionProps have following constructor, and first argument of them constroctor is not Props:

LambdaFunctionAction

constructor(func: lambda.IFunction) {}

CloudWatchLogsAction

constructor(logGroup: logs.ILogGroup, props: CloudWatchLogsActionProps) {}

S3PutObjectActionProps

constructor(bucket: s3.IBucket, props: S3PutObjectActionProps) {}

So if first question is "yes", I will do this refactoring in another PR!

No. Let's leave the constructors as they are now.

BTW, there's no point in having both CommonActionProps and CommonAwsActionProps for IoT Actions - you only need one common superinterface (it's a different story with CodePipeline Actions, but I don't want to get into the details of why that is here).

Should we have static, string-typed constants for these substitution templates? So that it's clear which ones can be used by which Actions?

I think we cannot define all static, string-typed constants. Because users can use MQTT payload property defined by themselves in template.

For example, there are following MQTT payload:

{
  "year": "2021",
  "month": "Nov",
  "day": "04",
  "data": 123.4
}

The user can define the bucket key as ${year}/${month}/${day}/${timestamp()}.

Are we on the same page?

I don't think we need to have all of them - but you have a few that are always available, right? Like ${timestamp()} and ${topic()}. Maybe it's worth it to include those that are always available, and just document the other ones?

OK, I also want to make it as user-friendly as possible. So I'd like to be on same page!

1. there are about 70 functions, and they take arguments

• So we need to use functions, instead of static constants.

2. there are also Operators.

• So it is better that our functions return without ${} as topic().

3. in many cases, they are used embedded in strings without any regularity. For example, cloudwatch metric name could be used substitution templates.

And I thought 3 plans.

PLAN1, implement a set of functions

In light of the above, it might be better for us to implement a set of functions and for users to use them in template literals, as following:

key: `\${${iot.Template.topic(2)}}/\${${iot.Template.timestamp()}}`

PLAN2, just validation

Or, is it better to implement validation as JsonPath of @aws-cdk/aws-stepfunctions
https://github.com/aws/aws-cdk/tree/master/packages/@aws-cdk/aws-stepfunctions/lib/fields.ts#L216-L226

For substitution templates, I think we will be using a lot of regular expressions.

PLAN3, implement Expression and Template

Or, we implement Expression and Template?

I mean Expression as following:

iot.Expression.substring(iot.Expression.topic(2), 1)
// return type of Expression

I mean Template as following:

iot.Template.format(
  '{}/{}',
  iot.Expression.substring(iot.Expression.topic(2), 5, 10),
  iot.Expression.timestamp(),
)
// return type of Template

And, properties that can be used substitution templates will have the type Template.

{
  key: iot.Template,
}

The above may help to naturally communicate to users that substitution templates are available.

Before I answer, I have one more clarifying question.

Are any of the substitutions Action-dependent? So, are there any substitutions that can only be used with certain Action(s)?

In any case, I see now this is a much more complicated topic than I initially thought, and should be tackled in a separate PR. Let's continue the discussion on how to best model them - that's fine, but I don't want to block this PR on the discussion. Let's proceed with this PR without anything around substitutions, like it is now.

Are any of the substitutions Action-dependent? So, are there any substitutions that can only be used with certain Action(s)?

As far as I can tell from the documentation, no. And from the documentation, they all seem to be common functions and not Action-dependent. 🙂

Let's proceed with this PR without anything around substitutions, like it is now.

Sounds good!

Are any of the substitutions Action-dependent? So, are there any substitutions that can only be used with certain Action(s)?

As far as I can tell from the documentation, no. And from the documentation, they all seem to be common functions and not Action-dependent. 🙂

Great! Given that, I would go with Plan 1 from #17307 (comment).

I think it's fine to make this public.

Show cannedAcl too?

So, where is it? 🙂

OK, I see below. In the future, I would merge these examples, because otherwise the ReadMe becomes way too long.

I don't like the name cannedAcl. Let's call this accessControl, like we do in CodePipeline's S3DeployAction.

Let's inline this method, it doesn't add much value.

Let's use cannedAclaccessControl here too.

Let's only assert for the presence of this Key (you might have to use the objectLike and arrayLike helpers to achieve this).

Beautiful!🤩

OK, I see below. In the future, I would merge these examples, because otherwise the ReadMe becomes way too long.