fix: Align cache configstore with framework implementation #9282
Conversation
|
/kind fix |
|
@softho0n: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@softho0n, thank you for the contribution ! Can you rebase the PR with the main branch and push again ? |
|
Yes! Of course. I will do it soon. |
|
@softho0n looks like tide is running now and isn't complaining about rebase anymore. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: waveywaves The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@waveywaves Thank you! |
|
Hi @waveywaves! I have a simple question. So, now, Is it right that I don't have to rebase? |
|
Oh! I think I need the lgtm label. 🤔 If you don't mind, can I ask you? |
|
@softho0n I spoke to @twoGiants about this PR and would like to relay one question he wanted to ask i.e. have you run this locally and checked if the configuration is picked up on changes to the configmap ? |
There was a problem hiding this comment.
Hi @softho0n ! Great job on this one 😸 👍
Could you do two more things and then we're good?:
- Bring the unit tests together. I think you can now have one
TestOnCacheConfigMapChangedwhich covers all the cases. - Remove the release note => set to
NONE. It's not a user facing change.
We would really need to test this manually too (as I discussed with @waveywaves ), but now that the annotation propagation is not implemented yet (only in my branch), you can't manually test it easily. You would have to look through the resolver logs and count cache hits and cache misses before and after you change the values in the caching config map. Wdys? Could you do that too or is it to much?
Thank again for the great work! 🥇
Remove the custom Config struct and NewConfigFromConfigMap function in the resolver cache package. Instead, use the standard map[string]string pattern and DataFromConfigMap from the resolution framework. This aligns the cache configuration handling with other Tekton resolvers for better consistency and maintainability. Fixes tektoncd#9275
Consolidate unit tests into TestOnCacheConfigMapChanged for better flow verification
296d083 to
8c7b984
Compare
|
Hi @twoGiants ! Thank you for the great feedback.
Thanks again for the great guidance! |
Remove the custom Config struct and NewConfigFromConfigMap function in the resolver cache package. Instead, use the standard map[string]string pattern and DataFromConfigMap from the resolution framework.
This aligns the cache configuration handling with other Tekton resolvers for better consistency and maintainability.
Fixes #9275
Changes
Configstruct in configstoreNewConfigFromConfigMapwith the framework's standardDataFromConfigMaponCacheConfigChangedto handle parsing logic directly frommap[string]string.Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes