-
Notifications
You must be signed in to change notification settings - Fork 153
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
Add function to find ConfigMaps and Secrets in manifests #5319
Conversation
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5319 +/- ##
=======================================
Coverage 25.28% 25.28%
=======================================
Files 444 444
Lines 47554 47572 +18
=======================================
+ Hits 12022 12028 +6
- Misses 34590 34601 +11
- Partials 942 943 +1 ☔ View full report in Codecov by Sentry. |
@@ -156,3 +156,16 @@ func findUpdatedWorkloads(olds, news []provider.Manifest) []workloadPair { | |||
} | |||
return pairs | |||
} | |||
|
|||
func findConfigs(manifests []provider.Manifest) map[provider.ResourceKey]provider.Manifest { |
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.
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.
Should we add a function comment like "findConfigs returns ConfigMap and Secret resources" to the head of this function? TBH, I don't actually like this implementation (even the original method) since we have to mention that this function returns both ConfigMap and Secret resources while its name makes the reader think it only returns config.
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.
@khanhtc1202
I got your point. How about renaming this as findConfigsAndSecrets
?
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.
That sounds nice 👍 @Warashi
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 renamed it and added a comment.
dedd8fc
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
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
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, thanks 👍
What this PR does:
This PR adds the function
findConfigs
to find ConfigMaps and Secrets in manifests.Why we need it:
To determine sync strategies, we need to find updated ConfigMaps or Secrets. This PR adds functionalities for finding these candidates.
Which issue(s) this PR fixes:
Part of #4980
Does this PR introduce a user-facing change?: No