Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reusable Package #52

Closed
bradkwadsworth-mw opened this issue Nov 21, 2023 · 8 comments
Closed

Reusable Package #52

bradkwadsworth-mw opened this issue Nov 21, 2023 · 8 comments

Comments

@bradkwadsworth-mw
Copy link

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
Copy link
Collaborator

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

@negz
Copy link
Member

negz commented Nov 22, 2023

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
Copy link
Author

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
Copy link
Collaborator

stevendborrelli commented Nov 22, 2023

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
Copy link
Member

negz commented Jan 2, 2024

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
Copy link
Member

negz commented Jan 8, 2024

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
Copy link
Author

@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
Copy link
Member

negz commented Feb 28, 2024

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.

@negz negz closed this as completed Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants