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

Fixed multiple full scan AWS buckets scanning deployment name conflict #128

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

samthesecret
Copy link

TITLE

Change Summary

PR Checklist

  • [ X] I've read and followed the Contributing Guide.
  • Documents/Readmes
    • Updated accordingly
    • [ X] Not required
  • Plugins that have versioning

Other Notes

This is resolution against the bug #121

@trend-jack-c-tang trend-jack-c-tang added the bug Something isn't working label Apr 17, 2023
@trend-jack-c-tang trend-jack-c-tang requested a review from a team April 17, 2023 10:24
Copy link
Contributor

@trend-jack-c-tang trend-jack-c-tang left a comment

Choose a reason for hiding this comment

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

  • Max bucket name length is 63. With prefix included, step function name length will exceed the max length which is 80. Please consider not giving state machine name, and output the name and ARN in the stack outputs for customers to use.
  • PR description needs to be completed: update title and change summary
  • The branch has unresolved conflict that needs to be resolved
  • Use conventional commit message for the commit: fix: Multiple full scan AWS buckets scanning deployment name conflict

@trend-jack-c-tang
Copy link
Contributor

The issue related to this PR should be #120

@trend-jack-c-tang trend-jack-c-tang linked an issue Apr 17, 2023 that may be closed by this pull request
@samthesecret
Copy link
Author

Thanks for the correction on bug number, I missed attaching the correct one.

I have resolved the conflict.

Shall I make change to control the step function length ?

@trend-jack-c-tang
Copy link
Contributor

Shall I make change to control the step function length ?

Yes, this should be addressed, or it won't work for longer bucket name. I'd suggest not giving default name for the step function. What do you think?

@samthesecret
Copy link
Author

I have made changes to make sure the length of StateMachine function should be in limit.

StateMachineName:
Fn::Join:
- ""
- - fullScanSSM-
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it SSM but not SM? How about we just remove the acronym, since we already know that's a state machine/step function? By the way, would you please also use capital for the first char, like ScannerLoop?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploy multiple scheduled scan plugin in same region
2 participants