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

flux plugin: switch to using flux native data types instead of unstructured #4112

Closed
gfichtenholt opened this issue Jan 19, 2022 · 4 comments · Fixed by #4175 or #4238
Closed

flux plugin: switch to using flux native data types instead of unstructured #4112

gfichtenholt opened this issue Jan 19, 2022 · 4 comments · Fixed by #4175 or #4238
Assignees
@gfichtenholt
Labels
component/apis-server Issue related to kubeapps api-server kind/enhancement An issue that reports an enhancement for an implemented feature
Projects
Kubeapps
Milestone
Pluggable support...

Comments

@gfichtenholt
Copy link
Contributor

Documenting leftover items for flux plugin per Pepe's request so we can prioritize it against other work

This is a leftover item from #3899

@antgamdia said:
Not now, but perhaps we can use the typed struct instead of dealing with the unstructured data directly (as I'm doing in the kapp plugin).

This has been on my TODO list for a while. To be clear. The benefit to end user is none. But the benefit to developers/maintainers of this code is huge. Code based on unstructured types is difficult to work with/maintain and is error prone. Not to mention very verbose.
Ideally, I'd want to switch to use APIs like https://github.com/fluxcd/source-controller/tree/main/api/v1beta1 which should make the code much more concise and easier to read.
@project-bot project-bot bot added this to Inbox in Kubeapps Jan 19, 2022
@gfichtenholt gfichtenholt self-assigned this Jan 19, 2022
@gfichtenholt gfichtenholt added component/apis-server Issue related to kubeapps api-server size/M labels Jan 19, 2022
@gfichtenholt gfichtenholt changed the title flux plugin: switch to using flux data types instead of unstructurred flux plugin: switch to using flux native data types instead of unstructurred Jan 19, 2022
@gfichtenholt gfichtenholt changed the title flux plugin: switch to using flux native data types instead of unstructurred flux plugin: switch to using flux native data types instead of unstructured Jan 19, 2022
@antgamdia
Copy link
Contributor

Totally agree, there is no direct user-faced benefit, but from a developer pov, the difference is huge. For the record, find below a screenshot comparing the two approaches:

image

@ppbaena ppbaena added this to the Pluggable support for Flux v2 milestone Jan 19, 2022
@ppbaena ppbaena moved this from Inbox to Committed in Kubeapps Jan 19, 2022
@gfichtenholt
Copy link
Contributor Author

gfichtenholt commented Jan 24, 2022 •
edited

Just FYI, I did see what Antonio did with kapp_controller and I am hoping to take things a bit further and least in flux plugin. In kapp_controller there are constant conversions from unstructured to typed (runtime.DefaultUnstructuredConverter.FromUnstructured) and back (runtime.DefaultUnstructuredConverter.ToUnstructured) due to having to work with dynamic.Interface behind the scenes to access server resources. dynamic.Interface forces one to use unstructured, therefore constant back and forth. Yes, what is there now a reasonable first step. But, for flux, I am hoping to eventually get rid of dependency on dynamic.Interface in production code and tests and replace it with something like https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.11.0/pkg/client#Client which will let me work with typed objects all the way from server to caller and back. This is a bit of work though. A lot of existing code (production and tests) will need to be refactored, but I feel it's worth the effort.

@absoludity
Copy link
Contributor

Excellent, that'd be great to be able to use a typed client without having to use the same client-go version as the upstream go lib (carvel/flux - that was what stopped us initially using the typed client for carvel, from memory, needing to update our own go.mod deps to match their version).

@ppbaena ppbaena moved this from Committed to Next iteration discussion in Kubeapps Jan 25, 2022
gfichtenholt added a commit to gfichtenholt/kubeapps that referenced this issue Jan 27, 2022
@gfichtenholt
first step towards issue
74b3744 
  flux plugin: switch to using flux native data types instead of unstructured vmware-tanzu#4112
@gfichtenholt gfichtenholt mentioned this issue Jan 27, 2022
Merged
@gfichtenholt gfichtenholt linked a pull request Jan 27, 2022 that will close this issue
First step for flux plugin: switch to using flux native data types instead of unstructured #4112 #4175
Merged
Kubeapps automation moved this from Next iteration discussion to Done Jan 31, 2022
gfichtenholt added a commit that referenced this issue Jan 31, 2022
@gfichtenholt
First step for flux plugin: switch to using flux native data types in…
1c23836 
…stead of unstructured #4112  (#4175)

* first step towards issue
  flux plugin: switch to using flux native data types instead of unstructured #4112

* fix merge issues

* minor fixes

* attempt to remove cmd/apprepository-controller/vendor/k8s.io/code-generator from my PR

* attempt #2

* fix go.sum

* still fixing go.mod and go.sum after merge from upstream main

* attempt to get the latest from cmd/apprepository-controller/vendor/k8s.io/code-generator

* revert last change: didn't work

* Antonio's feedback

* refactored clientgetter so that we can add more different client getters to it without breaking existing code
@gfichtenholt gfichtenholt reopened this Jan 31, 2022
Kubeapps automation moved this from Done to In progress Jan 31, 2022
@gfichtenholt
Copy link
Contributor Author

still work to do...

@ppbaena ppbaena added kind/enhancement An issue that reports an enhancement for an implemented feature priority/high labels Jan 31, 2022
gfichtenholt added a commit to gfichtenholt/kubeapps that referenced this issue Feb 9, 2022
@gfichtenholt
remaining work for flux plugin: switch to using flux native data type…
0980bd1 
…s instead of unstructured vmware-tanzu#4112 (part 1)
gfichtenholt added a commit to gfichtenholt/kubeapps that referenced this issue Feb 9, 2022
@gfichtenholt
flux plugin: switch to using flux native data types instead of unstr…
5fce810 
…uctured vmware-tanzu#4112 (part 2)
gfichtenholt added a commit to gfichtenholt/kubeapps that referenced this issue Feb 9, 2022
@gfichtenholt
flux plugin: switch to using flux native data types instead of unstr…
b333e62 
…uctured vmware-tanzu#4112 (part 3)
@gfichtenholt gfichtenholt mentioned this issue Feb 9, 2022
Merged
@gfichtenholt gfichtenholt linked a pull request Feb 9, 2022 that will close this issue
fix for httpclient library SetClientTLS cleanup + unit tests #4119 & finish flux plugin: switch to using flux native data types instead of unstructured #4112 #4238
Merged
gfichtenholt added a commit to gfichtenholt/kubeapps that referenced this issue Feb 9, 2022
@gfichtenholt
flux plugin: switch to using flux native data types instead of unstr…
6d67c09 
…uctured vmware-tanzu#4112 (part 5)
gfichtenholt added a commit to gfichtenholt/kubeapps that referenced this issue Feb 9, 2022
@gfichtenholt
flux plugin: switch to using flux native data types instead of unstr…
7fe15bc 
…uctured vmware-tanzu#4112 part 5
Kubeapps automation moved this from In progress to Done Feb 9, 2022
gfichtenholt added a commit that referenced this issue Feb 9, 2022
@gfichtenholt
fix for httpclient library SetClientTLS cleanup + unit tests #4119 & …
02bb375 
…finish flux plugin: switch to using flux native data types instead of unstructured #4112  (#4238)

* attempt #2

* fix for  httpclient library SetClientTLS cleanup + unit tests #4119
plus a couple unrelated unit test clean up items in flux plugin

* remove most files from PR to see if passes CI end-2-end tests

* add files back into PR, as they seem to have no effect on CI

* remaining work for  flux plugin: switch to using flux native data types instead of unstructured #4112 (part 1)

* flux plugin: switch to using flux native data types instead of unstructured #4112 (part 2)

* flux plugin: switch to using flux native data types instead of unstructured #4112 (part 3)

* flux plugin: switch to using flux native data types instead of unstructured #4112 (part 5)

* flux plugin: switch to using flux native data types instead of unstructured #4112 part 5

* minor fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gfichtenholt gfichtenholt

Labels
component/apis-server Issue related to kubeapps api-server kind/enhancement An issue that reports an enhancement for an implemented feature
Projects
No open projects
Kubeapps
  
Done
Milestone
Pluggable support for Flux v2
4 participants
@absoludity @ppbaena @antgamdia @gfichtenholt