I don't think this will work if there is a trailing / in basePath (e.g. https://foo.bar/) and I think it probably should, can you add a test case for that and validate

Thanks for your review, actually the trailing / is removed in ClientBuilder.build, so basePath would not have a trailing / (unless the user create a ApiClient directly without using ClientBuilder.build)

@dddddai this repo will stay vendor-neutral so i dont think it's a good idea to put such a test that verifying the behavior for rancher.

a better way to do this is to have an additional setter e.g. setAPIPrefix for the class GenericKubernetesApi so that users can tweak api path freely.

@yue9944882 Thanks for your review

this repo will stay vendor-neutral so i dont think it's a good idea to put such a test that verifying the behavior for rancher.

Actually this test just makes sure tweakCallForCoreV1Group work fine with api prefix, so I guess the function name should be like testApplyServiceWithApiPrefix?

a better way to do this is to have an additional setter e.g. setAPIPrefix for the class GenericKubernetesApi so that users can tweak api path freely.

But users could not see GenericKubernetesApi when using Kubectl, for example:

V1Service service =
    Kubectl.apply(V1Service.class)
        .apiClient(new ClientBuilder().setBasePath("http://localhost:" + 8384 + prefix).build())
        .resource(
            new V1Service()
                .kind("Service")
                .apiVersion("v1")
                .metadata(
                    new V1ObjectMeta()
                        .namespace("foo")
                        .name("bar")
                        .labels(Collections.singletonMap("app", "bar")))
                .spec(spec))
        .execute();

In this case, users can only set api prefix on ApiClient, shall I add the setter in ClientBuilder and ApiClient?

@dddddai IIUC this pull doesnt need to involve changes in the KubectlApply related sources. making GenericKuberentesApi able to respect the prefix from the ApiClient should be sufficient. so we can remove this test, are we?

@dddddai IIUC this pull doesnt need to involve changes in the KubectlApply related sources. making GenericKuberentesApi able to respect the prefix from the ApiClient should be sufficient. so we can remove this test, are we?

on the other hand, i think the usage of setBasePath here is error-proning b/c the setter is doing an in-place modification upon the ApiClient instance. there will be thread-safety issue if multiple generic api modifying the basepath concurrently. ideally, what i expected is a function to provide an immutable shallow copy of ApiClient before we tweak the prefix, such as:

ApiClient shallowCopied = apiClient.newBuilder().setBasePath("<path>" + prefix).build()

we can do this in a follow-up anyway

i think the usage of setBasePath here is error-proning b/c the setter is doing an in-place modification upon the ApiClient instance. there will be thread-safety issue if multiple generic api modifying the basepath concurrently

Sorry I didn't get your point, setBasePath sets the basepath of ClientBuilder for creating a new ApiClient object, each cluster has its own ApiClient.
And there is no method for GenericKuberentesApi to modify the basepath. Please correct me if I'm wrong.

oh oversight, i took ClientBuilder as ApiClient.