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

Merge Common Utils into opensearch-project/OpenSearch #114

Open
CEHENKLE opened this issue Feb 11, 2022 · 10 comments
Open

Merge Common Utils into opensearch-project/OpenSearch #114

CEHENKLE opened this issue Feb 11, 2022 · 10 comments
Labels
enhancement New feature or request

Comments

@CEHENKLE
Copy link
Member

Is your feature request related to a problem? Please describe.
Common Utils was stood up as an independent repo back in the days of ODFE. We have not revisited whether this still makes sense in an OpenSearch world :)

This issue is to evaluate if it makes sense to move common utils into core, and if so how/where.

Please note: Common Utils right now is a grab bag of different features and functions, many of which are marked deprecated. We'd want to make sure we were moving over only the things that made sense in a mindful manner, rather than a straight up "lift and shift" as is.

Require work:
Accounting and evaluation of what's currently in common utils.
For each item, determining the best location to move it into core (or into another plug in if applicable).

@CEHENKLE CEHENKLE added the enhancement New feature or request label Feb 11, 2022
@dblock dblock changed the title Review potential to merge Common Utils into core Merge Common Utils into core Feb 14, 2022
@dblock dblock changed the title Merge Common Utils into core Merge Common Utils into opensearch-project/OpenSearch Mar 21, 2022
@peternied
Copy link
Member

Common Utils has presented with ownership hiccups with everyone and no-one being responsible directly for it. I would prefer the clarity of putting it inside the more actively managed codebase

@nknize
Copy link

nknize commented Mar 21, 2022

We have not revisited whether this still makes sense in an OpenSearch world :)

Seems premature. I think this ☝️ should be resolved before any refactoring to core is done?

@dblock
Copy link
Member

dblock commented Mar 22, 2022

We have not revisited whether this still makes sense in an OpenSearch world :)

Seems premature. I think this ☝️ should be resolved before any refactoring to core is done?

It seems like merging it into core is a way to assign ownership. This may or may not be bad.

@nknize Do you have technical arguments not to?

@peternied
Copy link
Member

We have not revisited whether this still makes sense in an OpenSearch world :)

CommonUtils was a mechanism for all proto-OpenSearch codebases to shared code. In the era of OpenSearch, the utility of this code as a separate library has been minimized.

If someone proposed making a CommonUtils2.0 repository today I would vote against its creation.

@nknize
Copy link

nknize commented Mar 23, 2022

@nknize Do you have technical arguments not to?

Posted at opensearch-project/OpenSearch#2545 (comment)

Repost

👎 for 2.0.0 it won't be ready:

  1. There is a lot of legacy code that should be removed before refactoring to core
  2. There is kotlin code that we need to tie in w/ the build hooks
  3. The Integration Tests are Disabled w/ a note "Enable this after integration with security plugin is done" So why aren't we just integrating the appropriate classes with the security plugin?
  4. There is no bwc integration tests and that will take some time to work out in core.

👍 for a future release if targeting core makes sense. We might want to integrate with security first (or dissolve the implementations across other plugins) then integrate security with core rather than blindly integrate with core.

@CEHENKLE
Copy link
Member Author

@praveensameneni Can you share on this issue (or point us to another issue) your analysis of the state of common utils and your plan to clean up the issues @nknize pointed out?

Thanks!

@CEHENKLE
Copy link
Member Author

CEHENKLE commented Jun 7, 2022

@praveensameneni Hi, I'd like to pick this back up and dust it off. I think folding common utils into core is the right direction, but I don't want to do that until the items Nick mentioned are done. Are you actively working on that, and if so, what's the ETA?

@praveensameneni
Copy link
Member

@CEHENKLE , Just some background, we created common-utils for supporting fine grained access in different plugins which has been adopted by many plugins - alerting, Anomaly Detection, Index Management, Async search among others. Over time few other plugins added code into common-utils. In addition to the items that @nknize posted, it needs to be re-designed and this will impact several plugins.
We are not actively working on the re-design of common-utils.

@dblock
Copy link
Member

dblock commented Jun 23, 2022

I must point out that this existential issue was written on February 11th, which is the 42nd day of the year.

@raj-chak
Copy link

raj-chak commented Feb 3, 2023

  • @nknize
  • We believe that since common-utils is a library, any Integration tests written should be outside of common-utils in other plugins. We can remove any integration tests that are commented. Same with bwc tests.
  • The legacy code is slated to be removed only during the 3.0.0 release.
  • There is kotlin code that we need to tie in w/ the build hooks. This can be done, and I am currently in the process of extracting more information to get this done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants