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

Add an automatically generated API client #144

Closed
wants to merge 5 commits into from

Conversation

aecay
Copy link

@aecay aecay commented Aug 15, 2022

This PR is a first cut at adding a clientset for K6 objects that can be used programmatically in go (in the same way that vanilla k8s apis in v1/core, v1/batch, etc).

The PR is divided into 3 commits:

  1. Add and bump some dependencies
  2. Rename the api directory to apis. This is driven by an (AFAICS undocumented) assumption in the kube-codegen codebase that all APIs will be located in a single package, and will be named according to PARENT-PKG/GROUP/VERSION. So api/v1alpha doesn't work quite right in the codegen (this piece of code which does something surprising when a package name is exactly api doesn't help either....)
  3. Add the makefile rule to generate the code, as well as the generated code files.

There are a few outstanding questions I have:

  • What is the minimum version of the go compiler and the k8s api libraries that you want to support? I just built with go1.18 and bumped the deps as far as they would go, since this seemed like the easiest way to get it working. If you have specific minimum version support requirements/goals, then I can dial the versions back to meet them
  • How (if at all) would you like the code generation to be integrated in the CI/build process? So far, the target is in the makefile but nothing calls it (other than manually)
  • The integration with the kube-codegen library is a bit ad hoc. The official docs want the integrator to call a bash script that's bundled into an upstream go package (:exploding_head:). They also rely on having some libraries (such as kube-codegen) vendored into the integrating repo. Given that go run exists and this repo doesn't vendor its deps, the upstream advice seemed to me like not the best way to integrate the upstream library...

PS If you are wondering, "what's this for," at $WORK we have an internal tool that does supervision of NFT tests in a k8s cluster. So far we use vanilla k8s Jobs for this (that run k6 on the inside) but we want to migrate to the operator. We need programmatic access to the K6 resources in the cluster in in our test supervisor (written in go) so we can do things like "wait until the test has passed and then [push a docker image/deploy the version to a test env/etc]" I/we were able to get a certain distance with the generic REST interface, but at a certain point having properly typed API clients becomes very helpful....

@CLAassistant
Copy link

CLAassistant commented Aug 15, 2022

CLA assistant check
All committers have signed the CLA.

@yorugac
Copy link
Collaborator

yorugac commented Aug 20, 2022

Thank you for the PR @aecay! IMHO, this feature is valid, I'm even surprised it hasn't come up before. Just as a heads-up, I won't be able to properly review this PR for a couple of weeks. At first glance, it looks OK but it does change a lot of things around and generation seems intricate. It might also clash with #138 changes.

One request from my side now: please remove dependency updates (other than what is added by the client generation). Our policy is to do dependency updates in separate PRs and therefore, builds.

As for your questions:

  1. currently, I'd say it's 1.17.
  2. we only build docker images a the moment. I've been thinking of possibility to add manifests generation to the CI - and client-gen seems similar. But this is tied to the 3rd point too and can likely become very messy. Either way, changes to CI would be better put into a separate PR so no need to add anything here. Thanks for considering it.
  3. 🤔 what steps did you run exactly? Looking at this doc, that shell script is not meant to be used outside of Kubernetes repo. And in sample-controller, it looks like, well, a sample, not a requirement... Do they claim differently in another place? This might require going into more detail than I have capacity now but general advice would be to rely on Makefile if needs be. I.e. adding another command with the steps you took, etc. would be fine, so long as there're brief comments on what it actually does.

@yorugac yorugac added the enhancement New feature or request label Aug 20, 2022
@aecay
Copy link
Author

aecay commented Sep 1, 2022

Hi @yorugac . Thanks for your feedback (and sorry it has taken me a while to get back to this -- I needed to chase approvals for the CLA internally at my employer -- which I'm happy to say is now sorted out).

First of all, I've redone the branch with no dependency updates needed. 🎉 It turned out to be pretty simple (now that I know what I'm doing, more than I did when I started...)

  1. gotcha
  2. gotcha
  3. I've added the command needed to generate the api client files to the Makefile. I've added a comment explaining what that target does, and noting the fact that it's currently not hooked into any automation.

As far as the issue about vendoring of dependencies goes, as far as I can see the flags for client-gen to control the output file location are --output-base and --output-package. I tried passing --output-base . --output-package apis/k6/v1alpha1/generated which in principle should write the generated files into the right place in this repo. And it does -- but it generates the import paths between the generated modules like:

import (
	k6v1alpha1 "apis/k6/v1alpha1/generated/clientset/typed/k6/v1alpha1"
)

What we need that import path to be is actually github.com/k6-operator/apis/..... That just means that I need to generate the files into a folder named github.com/k6-operator, copy them out of there and into the right place in the source tree, and then delete the github.com folder. That's all handled in the makefile on my branch and it's not a big deal -- it just seemed like a strange usability quirk of the tool. I was worried I was missing something -- but now I think that upstream k8s just has a vendoring-heavy workflow and so has not catered for this situation. 🤷

@aecay
Copy link
Author

aecay commented Oct 7, 2022

Hi @yorugac . Just checking on this PR -- is there anything outstanding that I need to do from your POV? Thanks 🙂

@yorugac
Copy link
Collaborator

yorugac commented Oct 27, 2022

Hi @aecay, I've finally had some time to dig into your PR; my apologies for the delay.

Code wise: the current branch seems fine to me but I intend to run some more tests with it -- will let you know if anything comes up. Ideally, I'd ask to include an example of how clientset is used if it's not too much trouble. Our README is becoming kind of overwhelmed though, so maybe a separate folder docs/examples would be good.

However, I've been trying to understand myself what's going on with different Kubernetes generators 😂 Partially due to issues you raised about the quirks of the tooling but also because in this PR, we get a new dependency: previously we used basically kubebuilder - now it's also code-generator. And it appears that there are differences in opinion regarding clientset in ecosystem! Namely, this article and the issue kubernetes-sigs/kubebuilder#1152 are making it clear that kubebuilder approach is to go wtihout clientset.

Could you please share your opinion on this? I don't think we got this request about clientset from anyone before and I'd like to have a more clear view on 'why' we add this prior to merging 🙂 E.g. you mentioned hitting some limits at which you started needing a dedicated clientset: are there some specific requirements that couldn't be resolved with the default client or is it too much boilerplate, etc.?

@aecay
Copy link
Author

aecay commented Dec 1, 2022

@yorugac Thanks for this info. I was previously not familiar with this way of writing clients -- I had previously only used the approach with specific clientset per API. I am going to make an attempt at implementing our code with the kubebuilder method instead -- I think that will give us what we need! If it turns out that there are any issues encountered with kubebuilder, I may reopen this. But based on what I have seen so far, it should work out for us 🙂

Thanks again 🙂

@aecay aecay closed this Dec 1, 2022
@yorugac
Copy link
Collaborator

yorugac commented Dec 6, 2022

Thanks for the update and clarifying the situation, @aecay! I hope the operator works for your use case. And certainly feel free to re-open if needs be 🙂

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

Successfully merging this pull request may close these issues.

None yet

3 participants