Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Fix]: Let assignment of annotations to Pod without any existing ones #18

Merged
merged 7 commits into from
Dec 20, 2024

Conversation

nepomucen
Copy link
Contributor

What does this change resolve?

  • Fixes the following error:
    2024-12-18T11:36:46.057252474Z panic: assignment to entry in nil map
    which occurs when controller tries to set annotations to Pod without existing ones.

Open questions or TODOs before merging if any

Copy link

github-actions bot commented Dec 20, 2024

Go test coverage: 85.6% for commit 53da70e
View coverage for all packages
# Package Name                                                         | Coverage
+ github.com/NCCloud/metadata-reflector/internal/clients               |    88.1%
+ github.com/NCCloud/metadata-reflector/internal/common                |   100.0%
+ github.com/NCCloud/metadata-reflector/internal/controllers/reflector |    83.0%
+ github.com/NCCloud/metadata-reflector/internal/metrics               |   100.0%

@nepomucen nepomucen requested a review from BonySmoke December 20, 2024 11:20
Makefile Outdated Show resolved Hide resolved
@BonySmoke
Copy link
Contributor

BonySmoke commented Dec 20, 2024

@nepomucen great, thank you, this looks awesome! I wonder if we need to add the same for setting labels. Theoretically, there will be no case when the pod doesn't have labels because we use them to actually get the pod but to keep things error-free, let's add the same logic here, wdyt?

@BonySmoke BonySmoke added enhancement New feature or request bug Something isn't working labels Dec 20, 2024
@BonySmoke
Copy link
Contributor

Besides, I realized we need a similar logic for removing annotations and labels.
Maybe you could also add the logic for checking if labels/annotations exist here and here and if they don't exist, simply return that the pod was not updated.
Thank you, again!

@nepomucen
Copy link
Contributor Author

I agree with you about the extending the fix's logic to other functions as well.
It's done already, please review the changes again.

@BonySmoke
Copy link
Contributor

Thank you! Could you please cover these changes with tests as well 🙏

@nepomucen
Copy link
Contributor Author

Sure, will do ...

@nepomucen
Copy link
Contributor Author

The tests are now added.

Copy link
Contributor

@BonySmoke BonySmoke left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@BonySmoke BonySmoke merged commit 2b14207 into main Dec 20, 2024
2 checks passed
@BonySmoke BonySmoke deleted the fix/empty-metadata branch December 20, 2024 14:31
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants