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.

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

🛠️ Refactor metadata flattener #2345

Merged
merged 23 commits into from
Dec 4, 2023
Merged

Conversation

arybolovlev
Copy link
Contributor

@arybolovlev arybolovlev commented Nov 21, 2023

Description

This PR has the following changes:

  • Split the flattenMetadata function into 2 independent ones:
    • flattenMetadataFields -- to flatten all metadata fields, this one is used to flatten metadata of the datasources and template specs
    • flattenMetadata -- to cut some labels and annotations and then flatten metadata
  • Update all data sources read functions to be independent from corresponding resources, in this case, we can use different metadata flattenters
  • Update documentation to mention that data sources and templates will keep all annotations and labels, in spite of ignore_annotations and ignore_labels settings
  • Add a new function removeGeneratedLabels to remove job controller server-generated labels.

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

Release Note

Release note for CHANGELOG:

We have updated the logic of data sources and now the provider will return all annotations and labels attached to the object, in spite of the `ignore_annotations` and `ignore_labels` provider settings. In addition to that, a list of ignored labels when they are attached to `kubernetes_job(_v1)` and `kubernetes_cron_job(_v1)` resources were extended with labels `batch.kubernetes.io/controller-uid` and `batch.kubernetes.io/job-name` since they aim to replace `controller-uid` and `job-name` in the future Kubernetes releases.

References

Fix: #2336
Fix: #2340

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@arybolovlev arybolovlev marked this pull request as ready for review November 22, 2023 13:59
@arybolovlev arybolovlev requested a review from a team as a code owner November 22, 2023 13:59
@arybolovlev arybolovlev marked this pull request as draft November 22, 2023 15:02
@arybolovlev arybolovlev marked this pull request as ready for review November 23, 2023 12:46
@arybolovlev arybolovlev marked this pull request as draft November 23, 2023 16:50
@arybolovlev arybolovlev marked this pull request as ready for review November 28, 2023 08:04
@SarahFrench
Copy link
Member

I had a quick look and obviously don't have full context, but one thing I looked for but couldn't find was unit tests for the flattenMetadataFields and flattenMetadata functions. I did see that the acceptance tests check that data sources have one or more annotations/labels present, though. Is there any behaviour of those functions that could benefit from more explicit testing?

@arybolovlev arybolovlev force-pushed the refactor-metadata-flattener branch 3 times, most recently from 183e9fa to fc196ce Compare November 28, 2023 16:23
@arybolovlev
Copy link
Contributor Author

I had a quick look and obviously don't have full context, but one thing I looked for but couldn't find was unit tests for the flattenMetadataFields and flattenMetadata functions. I did see that the acceptance tests check that data sources have one or more annotations/labels present, though. Is there any behaviour of those functions that could benefit from more explicit testing?

I have added a unit test for the flattenMetadataFields function and working on the flattenMetadata one as we have discussed. Thanks!

Copy link

@jastr945 jastr945 left a comment

Choose a reason for hiding this comment

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

👍

@arybolovlev
Copy link
Contributor Author

@SarahFrench I have added a unit test for the flattenMetadata function. Big thank you for the idea and inspiration!

Copy link
Member

@SarahFrench SarahFrench left a comment

Choose a reason for hiding this comment

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

Thanks for adding those unit tests - they help me to understand the difference between the two functions. I'm glad that the two behaviours have been split apart!

Approving because the acceptance tests have passed and the unit tests look good and pass 🚀

kubernetes/structures_test.go Outdated Show resolved Hide resolved
Copy link
Member

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

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

Looks great!

.changelog/2345.txt Outdated Show resolved Hide resolved
//
// More information: https://kubernetes.io/docs/reference/labels-annotations-taints/
generatedLabels := []string{
"batch.kubernetes.io/controller-uid",
Copy link
Member

Choose a reason for hiding this comment

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

These first two are new. Could this be a breaking change if any existing configuration is depending on them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually discovered these two labels when switched Job and CronJob from flattenMetadata to flattenMetadataFields. Once I did it, tests were failing due to batch.kubernetes.io labels.

I think that should not cause a breaking change. In the original flattenMetadata function, we only made a conditional exception on annotations when a prefix was passed and kept cutting labels. These two labels were cut anyway.

@arybolovlev arybolovlev merged commit 76484fd into main Dec 4, 2023
13 of 14 checks passed
@arybolovlev arybolovlev deleted the refactor-metadata-flattener branch December 4, 2023 08:08
alexsomesan added a commit that referenced this pull request Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to get Kuberneted Ingress annotations data.kubernetes_nodes should include annotations
4 participants