resource and resourceList methods

[shawkins]

resource method offers quite a bit more logic stemming from NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicable over the regular HasMetadataOperation - deletingExisting, acceptVisitors, createOrReplaceAnd, the VisitFromServerWritable/CascadingDeletable interface, etc.

With the refactorings related to #3327 it would be straight-forward to express resource(item) as resources(item.getClass()).withItem(item) - but that would provide a regular ResourceOperationImpl / HasMetadataOperation, not a NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicable. The major functional difference is the expectation that inNamespace will update the item's namespace as well.

NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicable is of course conflicting with DryRunnable and Namespaceable - so ResourceOperationImpl can only partially implement it.

My question is - why is extra functionality (like createOrReplaceAnd) only available when using the resource method? What of it (like deletingExisting, acceptVisitors) is actually still needed? With a few minor changes I think it would eliminate the need for NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl and the related specialized handler logic.

[shawkins]

The preliminary changes I have for this look like https://github.com/fabric8io/kubernetes-client/compare/master...shawkins:afteroptions?expand=1 - which are layered on #3365

The idea is that we can simplify a lot of logic if we can more universally rely on the OperationContext for manipulating the operation returned from a resource handler. To do this I split apart the derived a context state from the base and was able to change all of the other logic dealing with ResourceHandlers to simply use the operation/resource.

[shawkins]

@manusa @rohanKanojia a related question to this topic - currently the resources(Class) method returns the basic ResourceOperationsImpl, but resource/resourceList will use the registered operation if possible. Should resources do the same?

captured #3378 to address this

[shawkins]

The current pr preserves the functionality of NamespaceVisit... classes. As for the api the dfferences with other logic:

• they default to the item namespace. If you specify withNamespace it overrides the item. This is different than the other dsl handling of the namespace. Could this be made more uniform?

• they support createOrReplaceAnd, deletingExisting, apply (which is deprecated), and accept

deletingExisting is really a different variation of createOrReplaceAnd - it could just be deleteAnd().createOrReplace().

Conceptually these are just a replacement for creating an intermediate variable:

var resourceList = client.resourceList(listToCreate).inNamespace(session.getNamespace());
resourceList.delete(); // the current logic also logs if delete returns false
var result = resourceList.create();

I would vote for deprecating createOrReplaceAnd and deletingExisting

accept doesn't fit with the other dsl - it applies visitors but results in a createOrReplace, whereas the rest of the dsl will produce a patch. I would vote for deprecating accept in favor of edit, which rather than just modifying the context item/items, would send the patch.

[shawkins]

On the namespacing I would eventually expect resource(item) to return an interface that extends Namespaceable<Resource> and Resource - that would require a minimal amount of logic to handle the explicit namespacing.

I've updated the pr with the initially proposed deprecations.

However VisitFromServerGetWatchDeleteRecreateWaitApplicable cannot implement Resource yet due to conflicts with DryRunable, Cascading, etc. It is a minor breaking change to make the interfaces compatible by introducing another generic type parameter:

CascadingDeletable would need to become: CascadingDeletable<T, D extends Deletable>, which modifies VisitFromServerWritable as well.

Then VisitFromServerGetWatchDeleteRecreateWaitApplicable could implement VisitFromServerWritable<T, EditReplacePatchDeletable>, which is compatible with Resource

[shawkins]

@manusa I've now tried to take what I was describing above to completion - and it won't work the way I'd like. It would need to also be invasive the other direction into things like EditReplacePatchDeletable, so I think the current set of deprecations is as much as I would do for now. The only open question would be whether to update the change log to warn resource(item) and resourceList(item) will evolve - or just live with the differing apis.

[manusa]

The only open question would be whether to update the change log to warn resource(item) and resourceList(item) will evolve - or just live with the differing apis

If I'm getting this right, the "will evolve" statement is just a speculation as of now. Since there are going to be more breaking changes and we don't have a clear time-frame, I don't thing that sending an uncertain warning is a very good idea, IMHO.

[cstaykov]

The CHEATSHEET still has the following entry:

Resource API

• Create And Wait until resource is ready:

Pod p = client.resource(pod1).createOrReplaceAnd().waitUntilReady(10, TimeUnit.SECONDS);

However, createOrReplaceAnd() is now deprecated. Can you provide an alternative usage that mimics the command above?

[rohanKanojia]

I think you should be able to do it by using intermediate variables as stated in javadoc:

Pod p = client.resource(pod1).createOrReplace()
p = client.resource(p).waitUntilReady(10, TimeUnit.SECONDS);

[cstaykov]

Thanks.

What is the difference between

client.pods().createOrReplace(pod1);

and

client.resource(pod1).createOrReplace()

[shawkins]

The difference currently is in the namespace handling.

client.resource(pod1).createOrReplace() - will assume that the resource should be created in the namespace specified on the resource. While the other assumes that it is the namespace associated with the context.