When would this not be a pod? If that can happen, shouldn't we return an error rather than ignoring the override data in that case?

createGeneratedObject gets called from two places:

1. Inside Run() which creates the pod
2. Inside generateService() which creates the service when you use the --expose arg

Those are the only two cases though.

This conditional is to control whether the override should be applied (Pod) or not (Service).

Previously, I had added a boolean parameter that was passed in to indicate whether to apply the override or not. @eddiezane suggested in a previous review comment checking Kind == Pod here instead, which I thought was a better approach because that information is available already in this function and it eliminates the need for an extra function parameter.

Let me know if this changes your thoughts above, or if you'd like to see a check in place and return an error, and I can add it to check that it is a Pod or Service and if not return an error. Also maybe a comment would help explaining why this check is performed.

Hmm, I see. What still bothers me is that the fact that this function needs to know the available overrides are not always valid, and the reason for that has to do with its callers and not with something fundamental about what it is using to make the distinction (pod vs service).

I know it's more verbose, but I'd be inclined to break up this big function into the three things it does, only two of which the Service code path wants, i.e.:

	obj, err := o.buildGeneratedObject(generator, names, params)
	if err != nil {
		return err
	}
	obj, err = o.ApplyOverride(obj, corev1.Pod{})
	if err != nil {
		return err
	}
	runObject, err := o.createGeneratedObject(f, cmd, obj)
	if err != nil {
		return err
	}

WDYT of that option?

Even there, it kinda bugs me to hardcode corev1.Pod{} at all, since the surrounding code seems to be making an effort to be generic.

Looking at the LookupPatchMeta options available in strategicpatch, I'm not sure how to get around that... unless we have openAPI available, maybe? Or there's some way to go from GVK (which we clearly have here) to Go struct? Or we could somehow get this override step right into the part that knows this is a pod, i.e. func (BasicPod) Generate?

Thanks Katrina. I agree with your point about not wanting to make conditional paths in the generic function based on specific resource types.

I did some thinking about what you said and I came up with a slightly different approach that leaves the existing function intact and I think will work.

I introduced an Overrider type which knows about the specific resource being overridden. An overrider can optionally be passed into createGeneratedObject(). So in the case of Pod creation, it is passed in, but for the service created when using --expose, it is nil.

What do you think of this idea? Does it satisfy your original concern that createGeneratedObject() should not be type checking?

It kinda bugs me that this function's signature needs to change to have so many parameters, and that which ones are relevant depends on overrideType. WDYT about moving the case statement inside your new Overrider#Apply, and having three separate, specialized util functions? That would also enable us to preserve the signature on the original, and to write doc comments that are easier to understand (the above comment should probably be expanded either way).

@KnVerey I know what you're saying. The new method signature was not good because it had arguments that only sometimes were used depending on the switch. The Overrider is what knows which type of override should be performed, so it should not make Merge have to know about each override type.

I updated the code to preserve the original Merge function, and moved the switch statement into the Overrider as you suggested. Then I created new separate methods for StrategicMerge and JsonPatch. It is a little bit more code, but the method signatures are cleaner and unit tests are simpler.

Regarding this TODO that was preexisting in the code...

// TODO: merge assumes JSON serialization, and does not properly abstract API retrieval

I'm not quite sure I get what this is asking. I understand that the function relies on JSON serialization, but I'm not sure what is "to do" about that or if it is even a problem. I'm also not sure what action is needed for "proper abstract API retrieval". Personally, I'd prefer to just remove TODO comments like this and put them in the backlog as an issue, but I am hesitant to do that without understanding the intent behind it and since it's been there several years.

Thanks again for your feedback so far, it's been helpful. And let me know if you have any other ideas for improving this.