Reusable Package - Githubissues

bradkwadsworth-mw commented 10 months ago

Any chance of moving the patching logic to an importable package? I would like to reuse the patching logic in a function that I am working on, but cannot because it is part of the main package. I could work on a PR if this is doable.

stevendborrelli commented 10 months ago

I think it would be a good idea, as other folks can use the logic in their own functions if needed.

negz commented 10 months ago

A few questions if we did this:

Would it make sense for the logic to live in this repo? Probably, especially given the plan to eventually deprecate the version that is currently in c/c.
What would the expectation be around maintenance of the library - e.g. would we as the function-patch-and-transform be signing up for significantly more work by maintaining P&T as a library as well?

bradkwadsworth-mw commented 10 months ago

My initial thoughts were that it would live in this repo in a subdir with a package name of patches or something like that. @negz your second point makes me wonder if it should live in a separate repo with its own unit tests and lifecycle.

stevendborrelli commented 10 months ago

I am leaning into a separate repo due to the sync issues we are starting to see with upstream P&T and concerns about deprecation. It would be nice if upstream, the P&T function and any forks share common code. We could ensure existing P&T users that the function behavior is equivalent.

negz commented 8 months ago

I still feel pretty bullish on removing the native P&T code from Crossplane. So personally I would not factor in needing to support a library used by both Crossplane core and this function.

I could imagine emulating native P&T support by transparently using a function inside of Crossplane.

negz commented 8 months ago

I'm wary of making a reusable patch and transform library. I'd like to hear more about how folks think it would be used.

My primary concerns are:

Maintaining a library is more involved than maintaining an "app" (in this case a function). If we expose code others can import, we have to start worrying about backward compatibility and breaking changes at the code level, not just the user-facing API level (i.e. function input schema).
Do we want to optimize for proliferation of P&T functionality? Of course today it can proliferate by either copying the code or forking this function. My hunch is that most of those forks would either be short-lived (e.g. to test out a new feature we may accept "upstream"), or fairly specialized (e.g. a fork to satisfy some private/unique requirement). Making P&T a library starts to feel like we expect there will be N function-patch-and-transform variants in use in the wild on an ongoing basis. Do we think that will be the case?

FWIW I don't think the fact that the code here duplicates the P&T code in c/c is a compelling reason to create a library. I'm also a little concerned about drift between the two implementations, but I hope to address that by freezing and one day removing the c/c P&T functionality.

bradkwadsworth-mw commented 8 months ago

@negz I would agree with you. I've since found alternative solutions to what I was trying to do that no longer requires P&T. I'd be fine with closing this if no one else has any issues.

negz commented 6 months ago

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

My understanding is that right now 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.

crossplane-contrib / function-patch-and-transform

Reusable Package #52