Skip to content

Feature/s3 jsonata support#658

Closed
mikewongblinx wants to merge 3 commits intoserverless-operations:masterfrom
blinxsolutions:feature/s3-jsonata-support
Closed

Feature/s3 jsonata support#658
mikewongblinx wants to merge 3 commits intoserverless-operations:masterfrom
blinxsolutions:feature/s3-jsonata-support

Conversation

@mikewongblinx
Copy link
Copy Markdown

No description provided.

@mikewongblinx mikewongblinx force-pushed the feature/s3-jsonata-support branch from 7c2d194 to 4e5efd6 Compare September 26, 2025 08:51
@zirkelc
Copy link
Copy Markdown
Collaborator

zirkelc commented Sep 26, 2025

Hey @mikewongblinx please add a test case for this.
You can chnage/extend one of the existing tests for S3.
here is an example covering both JSONPath and JSONata:

describe('should give step functions permission to * whenever StateMachineArn.$ (JSONPath) or {% $arn %} (JSONata) is seen', () => {
it('jsonpath', () => {
const stateMachineArn = 'arn:aws:states:us-east-1:123456789:stateMachine:HelloStateMachine';
const genStateMachine = id => ({
id,
definition: {
StartAt: 'A',
States: {
A: {
Type: 'Task',
Resource: 'arn:aws:states:::states:startExecution',
Parameters: {
'StateMachineArn.$': '$.arn',
Input: {},
},
Next: 'B',
},
B: {
Type: 'Task',
Resource: 'arn:aws:states:::states:startExecution.sync',
Parameters: {
StateMachineArn: stateMachineArn,
Input: {},
},
Next: 'C',
},
C: {
Type: 'Task',
Resource: 'arn:aws:states:::states:startExecution.waitForTaskToken',
Parameters: {
StateMachineArn: stateMachineArn,
Input: {},
},
End: true,
},
},
},
});
serverless.service.stepFunctions = {
stateMachines: {
myStateMachine1: genStateMachine('StateMachine1'),
},
};
serverlessStepFunctions.compileIamRole();
const statements = serverlessStepFunctions.serverless.service
.provider.compiledCloudFormationTemplate.Resources.StateMachine1Role
.Properties.Policies[0].PolicyDocument.Statement;
const stateMachinePermissions = statements.filter(s => _.includes(s.Action, 'states:StartExecution'));
expect(stateMachinePermissions).to.have.lengthOf(1);
expect(stateMachinePermissions[0].Resource).to.equal('*');
});
it('jsonata', () => {
const stateMachineArn = 'arn:aws:states:us-east-1:123456789:stateMachine:HelloStateMachine';
const genStateMachine = id => ({
id,
definition: {
QueryLanguage: 'JSONata', // JSONPath is default
StartAt: 'A',
States: {
A: {
Type: 'Task',
Resource: 'arn:aws:states:::states:startExecution',
Arguments: {
StateMachineArn: '{% $arn %}',
Input: {},
},
Next: 'B',
},
B: {
Type: 'Task',
Resource: 'arn:aws:states:::states:startExecution.sync',
Arguments: {
StateMachineArn: stateMachineArn,
Input: {},
},
Next: 'C',
},
C: {
Type: 'Task',
Resource:
'arn:aws:states:::states:startExecution.waitForTaskToken',
Arguments: {
StateMachineArn: stateMachineArn,
Input: {},
},
End: true,
},
},
},
});
serverless.service.stepFunctions = {
stateMachines: {
myStateMachine1: genStateMachine('StateMachine1'),
},
};
serverlessStepFunctions.compileIamRole();
const statements = serverlessStepFunctions.serverless.service.provider
.compiledCloudFormationTemplate.Resources.StateMachine1Role
.Properties.Policies[0].PolicyDocument.Statement;
const stateMachinePermissions = statements.filter(s => _.includes(s.Action, 'states:StartExecution'));
expect(stateMachinePermissions).to.have.lengthOf(1);
expect(stateMachinePermissions[0].Resource).to.equal('*');
});
});

@VirtueMe
Copy link
Copy Markdown
Collaborator

Hi @mikewongblinx — closing this PR for a few reasons:

No PR description. Every PR needs at minimum a summary of what it does and why. Without that, reviewers have no context for what problem is being solved or what the intended behaviour is.

No tests. As zirkelc noted back in September, test coverage is required. That request went unanswered.

Structurally conflicted. The #711 refactor moved all per-service IAM logic out of the monolithic compileIamRole.js into separate strategy modules. S3 permissions now live in iamStrategies/s3.js with tests in iamStrategies/s3.test.js. This PR modifies a file that no longer contains the relevant code.

If you'd like to reopen this as a fresh PR against the current structure, we'd welcome it — but it would need a clear description, tests covering both JSONPath and JSONata cases (see the existing s3.test.js for the pattern), and changes targeting iamStrategies/s3.js.

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.

3 participants