-
Notifications
You must be signed in to change notification settings - Fork 8
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
CSS-6701 Rename User
to Identity
#1133
CSS-6701 Rename User
to Identity
#1133
Conversation
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
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.
LGTM thank you for this - more LOC than i expected :)
As a bonus, it might be nice to fix the dockercompose too..
And the answer to other questions is, yes.
@@ -20,7 +20,7 @@ const ( | |||
// Minor is the minor version of the model described in the dbmodel | |||
// package. It should be incremented for any change made to the | |||
// database model from database model in a released JIMM. | |||
Minor = 5 | |||
Minor = 6 |
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.
it could be argued that this is a major version change.. i'll leave it to you to decide.
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.
I asked this question the other day, and I'm not sure which is the right way to go. But, an argument in favour of keeping it as a minor version bump, is that if consider it as a major change then it shouldn't rely on the existing schema and therefore we should create things from scratch (I mean, creating all tables, not just altering them). Since we're just applying the migration on the existing database schema, I think it's a bit more accurate to call it a minor change. But as I said, I'm open to discussion on the other approach.
Signed-off-by: Babak K. Shandiz <[email protected]>
Renamed UserModelDefaults to IdentityModelDefaults.
…s-03 Fixes for identitymodeldefaults.
ALTER TABLE IF EXISTS user_model_defaults RENAME TO identity_model_defaults; | ||
|
||
-- TODO (CSS-6701): Do we need to rename these instances as well? | ||
-- - ALTER TABLE IF EXISTS controllers RENAME COLUMN admin_user TO admin_identity_name; |
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.
I think we should rename this.
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.
The reason I didn't touch this one, is because there's another column in the same table, named admin_pass
. So, I thought this is a username/password pair instance, rather than an identity as we're speaking of. Do you still think this should be changed?
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.
I renamed it since @alesstimec also agrees with your suggestion.
|
||
-- TODO (CSS-6701): Do we need to rename these instances as well? | ||
-- - ALTER TABLE IF EXISTS controllers RENAME COLUMN admin_user TO admin_identity_name; | ||
-- - ALTER TABLE IF EXISTS audit_log RENAME COLUMN user_tag TO identity_tag; |
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 too (but we shouldn't update the JSON unless we inform web team because that will break api compatability).
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.
Yeah, I left the JSON and API spec as is.
-- TODO (CSS-6701): Do we need to rename these instances as well? | ||
-- - ALTER TABLE IF EXISTS controllers RENAME COLUMN admin_user TO admin_identity_name; | ||
-- - ALTER TABLE IF EXISTS audit_log RENAME COLUMN user_tag TO identity_tag; | ||
-- - ALTER INDEX IF EXISTS idx_audit_log_user_tag RENAME TO idx_audit_log_identity_tag |
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.
And update this.
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.
Done.
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 file name is misspelled, idenity -> identity
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.
lgtm
Use default database (`postgres`) for Candid
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
9598679
into
canonical:feature-serviceaccounts
Description
This PR renames the
User
entity (in theinternal/dbmodel
package) toIdentity
.Some notes/questions regarding this PR:
UserModelDefaults
toIdentityModelDefaults
? (not renamed yet).1_6.sql
, do we need to rename these two instances as well? (not renamed).controllers.admin_user
(toadmin_identity_name
).audit_log.user_tag
(toidentity_tag
).Fixes CSS-6701
Engineering checklist
Check only items that apply