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.

Sign up for GitHub

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

Jump to bottom

fix(k8sprocessor): fix metadata dependencies #513

Merged
merged 2 commits into from
Apr 1, 2022
Merged

fix(k8sprocessor): fix metadata dependencies #513

merged 2 commits into from
Apr 1, 2022

Conversation

andrzej-stencel
Copy link
Contributor

Fixes #512

@github-actions github-actions bot added the go label Mar 31, 2022
@andrzej-stencel andrzej-stencel force-pushed the k8sprocessor-metadata-deps branch 3 times, most recently from 5f399a2 to 66d28c1 Compare March 31, 2022 12:23
@andrzej-stencel andrzej-stencel marked this pull request as ready for review March 31, 2022 13:07
@andrzej-stencel andrzej-stencel requested a review from a team as a code owner March 31, 2022 13:07
pmalek-sumo
pmalek-sumo reviewed Mar 31, 2022
View reviewed changes
Copy link
Contributor

@pmalek-sumo pmalek-sumo left a comment

Choose a reason for hiding this comment

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

One comment w.r.t to the tests is that I'd maybe be more generous with comments, i.e. add comments where we're testing this particular feature that is added in this PR so that we know what's being tested.

pkg/processor/k8sprocessor/kube/client_test.go Show resolved Hide resolved
pkg/processor/k8sprocessor/kube/owner_test.go Outdated Show resolved Hide resolved
@pmalek-sumo
Copy link
Contributor

One comment w.r.t to the tests is that I'd maybe be more generous with comments, i.e. add comments where we're testing this particular feature that is added in this PR so that we know what's being tested.

ping

@andrzej-stencel andrzej-stencel force-pushed the k8sprocessor-metadata-deps branch from 74f99e4 to 9760d25 Compare April 1, 2022 08:18
@andrzej-stencel
Copy link
Contributor Author

One comment w.r.t to the tests is that I'd maybe be more generous with comments, i.e. add comments where we're testing this particular feature that is added in this PR so that we know what's being tested.

ping

Here's how I see it: there was a functionality that was not covered by tests (retrieving metadata for deployments and cronjobs), and this functionality didn't work correctly. I fixed the bug and added the tests for this functionality at the same time. I don't think there is anything else required with regards to this bug.

pmalek-sumo
pmalek-sumo approved these changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@pmalek-sumo pmalek-sumo left a comment

Choose a reason for hiding this comment

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

🤷

@andrzej-stencel andrzej-stencel enabled auto-merge (rebase) April 1, 2022 11:12
@andrzej-stencel andrzej-stencel merged commit 071308c into main Apr 1, 2022
@andrzej-stencel andrzej-stencel deleted the k8sprocessor-metadata-deps branch April 1, 2022 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmalek-sumo pmalek-sumo pmalek-sumo approved these changes

Assignees
No one assigned
Labels
go
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[k8sprocessor] implicit dependencies between owner metadata
2 participants
@andrzej-stencel @pmalek-sumo