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

Mark deprecated builtins Removed #18039

Merged
merged 16 commits into from
Jan 9, 2023
Merged

Mark deprecated builtins Removed #18039

merged 16 commits into from
Jan 9, 2023

Conversation

mpalmi
Copy link
Contributor

@mpalmi mpalmi commented Nov 18, 2022

This PR drops the logical database builtins for 1.13. This might need a bit more polish in the form of Factory stubbing, but getting it into CI to get some free testing.

@mpalmi mpalmi force-pushed the handle-shadowed-builtins branch 2 times, most recently from a43dc69 to 3dea37d Compare November 29, 2022 17:09
@mpalmi mpalmi force-pushed the remove-database-builtins branch from d91812e to ea6b5e6 Compare November 30, 2022 15:42
@mpalmi mpalmi force-pushed the handle-shadowed-builtins branch from 7c4064a to cb4460a Compare November 30, 2022 20:32
@mpalmi mpalmi force-pushed the remove-database-builtins branch from ea6b5e6 to 1eb63d8 Compare December 1, 2022 15:13
@mpalmi mpalmi changed the title Remove logical database builtins Mark deprecated builtins Removed Dec 1, 2022
@mpalmi mpalmi marked this pull request as ready for review December 1, 2022 15:15
@mpalmi mpalmi requested a review from a team December 1, 2022 15:15
@calvn
Copy link
Contributor

calvn commented Dec 1, 2022

It looks like TestSecretsEnableCommand_Run is consistently failing so we might have to update that test.

@mpalmi
Copy link
Contributor Author

mpalmi commented Dec 1, 2022

It looks like TestSecretsEnableCommand_Run is consistently failing so we might have to update that test.

Thanks @calvn! I'm looking into it right now, but I suspect it's related to the branch that this is based on. Updates coming soon.

@mpalmi mpalmi force-pushed the remove-database-builtins branch from 3cbf5b3 to 0b3689c Compare December 1, 2022 17:35
@calvn
Copy link
Contributor

calvn commented Dec 1, 2022

Could you do a go mod tidy to see if it changes the dependencies? I feel that removing 8k lines of code would allow us to remove some deps and/or upgrade others.

@mpalmi
Copy link
Contributor Author

mpalmi commented Dec 1, 2022

Could you do a go mod tidy to see if it changes the dependencies? I feel that removing 8k lines of code would allow us to remove some deps and/or upgrade others.

I thought about that and dropped this change, but on second thought, we can probably get rid of the mgo dep:

$  git diff
diff --git a/go.mod b/go.mod
index 2af3b42184..0517d8a803 100644
--- a/go.mod
+++ b/go.mod
@@ -199,7 +199,6 @@ require (
        google.golang.org/grpc v1.50.1
        google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0
        google.golang.org/protobuf v1.28.1
-       gopkg.in/mgo.v2 v2.0.0-20180705113604-9856a29383ce
        gopkg.in/ory-am/dockertest.v3 v3.3.4
        gopkg.in/square/go-jose.v2 v2.6.0
        k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9
diff --git a/go.sum b/go.sum
index f277488de2..2215696078 100644
--- a/go.sum
+++ b/go.sum
@@ -2594,8 +2594,6 @@ gopkg.in/ini.v1 v1.66.2 h1:XfR1dOYubytKy4Shzc2LHrrGhU0lDCfDGG1yLPmpgsI=
 gopkg.in/ini.v1 v1.66.2/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k=
 gopkg.in/jcmturner/goidentity.v3 v3.0.0 h1:1duIyWiTaYvVx3YX2CYtpJbUFd7/UuPYCfgXtQ3VTbI=
 gopkg.in/jcmturner/goidentity.v3 v3.0.0/go.mod h1:oG2kH0IvSYNIu80dVAyu/yoefjq1mNfM5bm88whjWx4=
-gopkg.in/mgo.v2 v2.0.0-20180705113604-9856a29383ce h1:xcEWjVhvbDy+nHP67nPDDpbYrY+ILlfndk4bRioVHaU=
-gopkg.in/mgo.v2 v2.0.0-20180705113604-9856a29383ce/go.mod h1:yeKp02qBN3iKW1OzL3MGk2IdtZzaj7SFntXj72NppTA=
 gopkg.in/natefinch/lumberjack.v2 v2.0.0/go.mod h1:l0ndWWf7gzL7RNwBG7wST/UCcT4T24xpD6X8LsfU/+k=
 gopkg.in/ory-am/dockertest.v3 v3.3.4 h1:oen8RiwxVNxtQ1pRoV4e4jqh6UjNsOuIZ1NXns6jdcw=
 gopkg.in/ory-am/dockertest.v3 v3.3.4/go.mod h1:s9mmoLkaGeAh97qygnNj4xWkiN7e1SKekYC6CovU+ek=

@mpalmi mpalmi force-pushed the remove-database-builtins branch from a468f22 to 4ee5fbf Compare December 2, 2022 20:45
@mpalmi mpalmi changed the base branch from handle-shadowed-builtins to main December 2, 2022 20:46
@mpalmi mpalmi added this to the 1.13.0-rc1 milestone Jan 9, 2023
@mpalmi mpalmi merged commit 31772c9 into main Jan 9, 2023
@mpalmi mpalmi deleted the remove-database-builtins branch January 9, 2023 14:16
AnPucel pushed a commit that referenced this pull request Jan 14, 2023
* Remove logical database builtins

* Drop removed builtins from registry keys

* Update plugin prediction test

* Remove app-id builtin

* Add changelog
AnPucel pushed a commit that referenced this pull request Feb 3, 2023
* Remove logical database builtins

* Drop removed builtins from registry keys

* Update plugin prediction test

* Remove app-id builtin

* Add changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants