-
Notifications
You must be signed in to change notification settings - Fork 156
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
[Lambda] Support Drift Detection for Lambda #5186
Conversation
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5186 +/- ##
==========================================
- Coverage 22.93% 22.85% -0.09%
==========================================
Files 419 420 +1
Lines 44986 45247 +261
==========================================
+ Hits 10318 10340 +22
- Misses 33873 34111 +238
- Partials 795 796 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented about sorting maps.
I think the code I commented on is not needed because it does not work as expected, but the test you wrote is passed.
// sortMap sorts the given map by keys and returns a new map. | ||
func sortMap(m map[string]string) map[string]string { | ||
keys := make([]string, 0, len(m)) | ||
for k := range m { | ||
keys = append(keys, k) | ||
} | ||
slices.Sort(keys) | ||
|
||
sorted := make(map[string]string) | ||
for _, k := range keys { | ||
sorted[k] = m[k] | ||
} | ||
return sorted | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The for-range for map[string]string is always looped in random order.
So, this code may not work as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood, what a meaningless code... 😅
let me re-think...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@ffjlabo Would you please review again? This is the only one change: #5186 (comment) |
Sorry for the rough review🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
What this PR does / why we need it:
Supported DriftDetection for Lambda
When OutOfSync:
Restrictions:
source
s3Bucket
s3Key
s3ObjectVersion
Which issue(s) this PR fixes:
Fixes #5182
Does this PR introduce a user-facing change?: Users can see drifts.
Note:
To use this feature, users need to add the following IAM Actions to their Piped's IAM Role.
Later I'll describe the details in another PR.