refactor: Move logic into pkg/ to be imported elsewhere

[MisterMX]

Fixes #52

To reuse parts of the logic in this function elsewhere in another project the go files need to moved from the main package into another package since Go does not allow imports of main packages ("programs").

This moves all function logic into pkg/fn/ and the main.go into cmd/fn/.

It also adds a NewFunction constructor with WithLogger option to circumvent the fact that log is a private field of Function (which is a bit cleaner and more function-al, anyways).

[negz]

Do you have an example of where you'd like to use this logic?

Making this package public implies that it becomes a library others can depend on. Is this definitely something the maintainers of this function want to support? It's something we resisted doing for a long time in the c/c equivalent that this code duplicates (and that I hope to one day delete, per crossplane/crossplane#4746).

[negz]

I just remembered #52. Edited the issue to link it.

[MisterMX]

Do you have an example of where you'd like to use this logic?

I would like to use it together with https://github.com/MisterMX/crossplane-function-server.

Making this package public implies that it becomes a library others can depend on. Is this definitely something the maintainers of this function want to support?

I understand the maintenance burden this might imply. Though I think it would not be that big of a problem since there is no need to guarantee API stability for it.

I made some adjustments to my changes, so only the logic files are moved and main.go stays in /.

[phisco]

I don't have strong opinions on the topic, the latest version with main.go in the root as in the template repository looks good to me, so I could go either way. I'll leave to you, @negz, the last call on whether we want this to be a public facing API.

[negz]

I don't feel comfortable exposing this as a public package.

Though I think it would not be that big of a problem since there is no need to guarantee API stability for it.

I look at this through the lens of https://www.hyrumslaw.com. Sure we could state that there's no guarantee of API stability, but how will that hold up when folks start depending on it? Do we end up in a situation where we're forced to either commit to stability, or upset people?

Another consideration is that if we have "lib-patch-and-transform" imported by N consumers, do we end up with an even more sprawling P&T featureset? Would we see PRs opened here for functionality that other functions/P&T implementations wanted, even if this function didn't want to use them?

I do sympathize that this is a tough one. Even if we assume that there's a path somehow removing native P&T from Crossplane, I expect there to be more than one function using P&T. For example in #26 I've proposed that folks should fork this function to experiment with new functionality. An explosion of forks has its own set of problems (though I'm not sure being able to import lib-patch-and-transform would actually help much with that).

Perhaps you could help me feel more comfortable with opening this package up by talking a little more about why you prefer being able to import this code in your server, as opposed to forking it?

[MisterMX]

Perhaps you could help me feel more comfortable with opening this package up by talking a little more about why you prefer being able to import this code in your server, as opposed to forking it?

Of course I could also use my fork - in fact I am actually doing that right now. But I would like to avoid that kind of fragmentation.

I look at this through the lens of hyrumslaw.com. Sure we could state that there's no guarantee of API stability, but how will that hold up when folks start depending on it? Do we end up in a situation where we're forced to either commit to stability, or upset people?

AWS Go SDK has a private package where they have their internal implementations but other folks can still import it while keeping the risk. Is this is a possible solution?

That being said, I actually only need a public constructor and the data the functions expects. My function call is actually very short:

	req := fnapi.RunFunctionRequest{
		Input:    customInput,
		// ...
	}
	fn := fnpt.NewFunction(
		fnpt.WithLogger(e.log),
	)
	res, err := fn.RunFunction(ctx, &req)

So only the NewFunction constructor and its options are necessary. Everything else can stay in an internal package.

[negz]

I actually only need a public constructor and the data the functions expects.

This is a step in the right direction. I'm still reluctant though.

I like that exposing only a constructor eliminates the concern about maintaining (code level) API compatibility for 'internals' like the Apply methods etc. On the other hand if I imagine a future where there are N functions all using P&T with slight differences, won't anyone who wants to add new functionality to P&T then need to either add it here (and thus have it eventually picked up by all functions) or fork?

[MisterMX]

On the other hand if I imagine a future where there are N functions all using P&T with slight differences, won't anyone who wants to add new functionality to P&T then need to either add it here (and thus have it eventually picked up by all functions) or fork?

My intention was to use this function as-is but call the function programatically: Instead of writing the P&T logic in the composition directly, I would have read it from an embed.FS and pass it as a Go struct.

Do you have an example of such changes and adoptions for specific use cases that wouldn't come up with the intended way of using this function from a composition - since the input API is already public?

[MisterMX]

@negz @phisco I rebased the branch on the latest master. Are there any chances to get this merged?

This would be great for us since we wouldn't have to rely on a custom fork anymore.

[negz]

@MisterMX After thinking about it for a while I still don't feel good about proceeding here, sorry.

My understanding is that the author of #52 no longer needs this. As far as I know that leaves only one known consumer that wants this change (https://github.com/MisterMX/crossplane-function-server). Given that, I don't want to change the contract of this function for now (i.e. for it to become a library).

If we find that there ends up with more than a small handful of forks of this function we can revisit.