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

feat: be able to import any k8s obj as source #1321

Closed
wants to merge 1 commit into from

Conversation

guilhem
Copy link

@guilhem guilhem commented Dec 22, 2024

This pull request introduces several significant changes to the kustomization_controller to enhance the discovery and handling of source references, as well as adding new tests to ensure functionality. The most important changes include adding a DiscoveryClient to the KustomizationReconciler, refactoring the source retrieval logic, and adding a new test for the SourceRefAPIVersion.

Enhancements to KustomizationReconciler:

  • Added DiscoveryClient to the KustomizationReconciler struct for dynamic API discovery. (internal/controller/kustomization_controller.go)
  • Introduced the Source type and refactored the getSource method to use unstructured.Unstructured for dynamic API version handling. (internal/controller/kustomization_controller.go)

Codebase updates:

  • Updated imports to include discovery package in multiple files. (internal/controller/kustomization_controller.go, internal/controller/suite_test.go, main.go) [1] [2] [3]
  • Modified the main function to initialize DiscoveryClient and pass it to the KustomizationReconciler. (main.go)

Testing improvements:

  • Added TestKustomizationReconciler_SourceRefAPIVersion to verify the correct handling of SourceRef API versions. (internal/controller/kustomization_controller_test.go)
  • Updated the test suite setup to include DiscoveryClient. (internal/controller/suite_test.go)

@guilhem guilhem force-pushed the anySource branch 2 times, most recently from f500771 to 4575da1 Compare December 22, 2024 17:58
This permit any object as source

Co-authored-by: Alan Amoyel <[email protected]>
Co-authored-by: bitswalk <[email protected]>
Signed-off-by: Guilhem Lettron <[email protected]>
@matheuscscp
Copy link
Member

Hi @guilhem, thanks for this PR. This change is far more complex than this PR sounds and must be discussed in the Flux dev meetings, which are open. Feel free to join one of the meetings in the new year if you want to discuss about this change: https://fluxcd.io/community/#meetings

@guilhem
Copy link
Author

guilhem commented Dec 22, 2024

Hi @guilhem, thanks for this PR. This change is far more complex than this PR sounds and must be discussed in the Flux dev meetings, which are open. Feel free to join one of the meetings in the new year if you want to discuss about this change: https://fluxcd.io/community/#meetings

Thanks @matheuscscp and after some time and reflection, I don't even think it's a good way to do it.
The main problem is with watching resources for reconciliation, and I don't see any simple way to solve it.
The other is with spec/status standardisation. Status.Artifact is not really something I like.

In fact, I tried to keep this PR as small as possible, but I think there is a far better way to do:
Having a dedicated "artifact" and "artifact set" objects.

But let's talk about it in a meeting. I can also talk about why we try to have something like that :)

@guilhem
Copy link
Author

guilhem commented Dec 22, 2024

But it was a good way to understand links between objects ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants