-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Make API not depend on SDK #18962
Make API not depend on SDK #18962
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 this is great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks fine. I like Tom's comment about highlighting the duplication (when functions/logic is involved).
I pointed out an apparent change in behaviour, that I'm not sure if it's important or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks!
The PR didn't build, there was a further change needed. I could've made that change smaller by casting between the consts versions of plugintypes and the api version, but this felt more appropriate since most of the code in question was then talking to api plugin methods. Happy to revisit that decision if this seems unpalatable to folks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM 👍 agreed on the last commit
…f plugintypes. We have to use the branch version of the api for this for now, until that PR is merged, but we can't wait because we're blocking that PR without this change.
…lure. Once this PR is merged, that one can be merged, and then we can remove the temporary branch versions for stepwise in vault and for api in stepwise.
…f plugintypes. We have to use the branch version of the api for this for now, until that PR is merged, but we can't wait because we're blocking that PR without this change. (#4)
* Initial commit * Import stepwise from current Vault snapshot * Update deps * Update dependencies for Vault 1.9 * Update containerd to fix issue with docker libs - docker brought in a broken containerd version v1.3.4 which resulted in an `undefined` error. * [COMPLIANCE] Update MPL 2.0 LICENSE * Unbreak hashicorp/vault#18962 by transitioning away from local copy of plugintypes. We have to use the branch version of the api for this for now, until that PR is merged, but we can't wait because we're blocking that PR without this change. (#4) * update dependencies * fix vault docker image * Fix deps to rewrite hashicorp/vault to openbao/openbao Signed-off-by: Alexander Scheel <[email protected]> * Use imported sdk/helper/stepwise over vault-testing-stepwise Note that this package is in the SDK so it remains importable by third-party plugins wishing to test against our instance. However, this allows it to be maintained alongside our distribution and is one fewer external dependency to maintain. Signed-off-by: Alexander Scheel <[email protected]> --------- Signed-off-by: Alexander Scheel <[email protected]> Co-authored-by: Jeff Mitchell <[email protected]> Co-authored-by: Ben Ash <[email protected]> Co-authored-by: Ben Ash <[email protected]> Co-authored-by: hashicorp-copywrite[bot] <[email protected]> Co-authored-by: Caleb Albers <[email protected]> Co-authored-by: Nick Cabatoff <[email protected]> Co-authored-by: Raymond Ho <[email protected]> Co-authored-by: Raymond Ho <[email protected]> Co-authored-by: JM Faircloth <[email protected]> Co-authored-by: John-Michael Faircloth <[email protected]>
This will allow api users to have fewer dependencies, and will allow sdk to depend on api. The cost is a little bit of duplication of code/types/constants but it seems like a reasonable trade-off.