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

Moving out legacy schemas into their own folder #1866

Merged
merged 9 commits into from
Feb 17, 2023

Conversation

aasimkhan30
Copy link
Contributor

@aasimkhan30 aasimkhan30 commented Feb 16, 2023

@alanrenmsft
Copy link
Contributor

where is the requirement coming from? is it appropriate to call them "legacy"?

@aasimkhan30
Copy link
Contributor Author

@alanrenmsft, drew had mentioned about it here. microsoft/vscode-mssql#17543 (comment)

IsNotFilter = true,
Values = new List<object> { "db_securityadmin" },
});
return filters;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to assemble the list of filters outside the getter and just return it immediately without needing to assemble every time?

@Charles-Gagnon
Copy link
Contributor

I would go with his suggestion of just putting them at the end instead of in a separate folder to start with then, unless we get specific feedback that people want those hidden.

The term "legacy" is definitely questionable and would be something we'd want to run by the engine owners first I'd think to make sure they're ok with the term. So for now just moving them to the end is likely fine for our purposes.

@Charles-Gagnon
Copy link
Contributor

@dzsquared Thoughts on this?

});
}
if (WorkspaceService<SqlToolsSettings>.Instance.CurrentSettings.SqlTools.ObjectExplorer.GroupBySchema)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting looks odd.

@dzsquared
Copy link

"Built-in schemas" instead of legacy schemas, matches the docs
I'm ok with them going into a folder or at the end - @erinstellato-ms

@Charles-Gagnon
Copy link
Contributor

Charles-Gagnon commented Feb 17, 2023

Wouldn't built-in then also want to include system schemas like dbo?

Which could be a consideration. Although I think if we're doing this it would be worth seeing about having a different folder icon (folder with schema icon as a badge?) - right now it blends in too well with the other folders IMO.

@aasimkhan30
Copy link
Contributor Author

Wouldn't built-in then also want to include system schemas like dbo?

Which could be a consideration. Although I think if we're doing this it would be worth seeing about having a different folder icon (folder with schema icon as a badge?) - right now it blends in too well with the other folders IMO.

@Charles-Gagnon , that's why I did not use 'System schemas' because that would also include dbo, sys, information schema.

@aasimkhan30
Copy link
Contributor Author

aasimkhan30 commented Feb 17, 2023

Should I use a neutral term like 'Other schemas'? Putting them towards the end would be a lot of work since our code does not support sorting OE elements as of now. @Charles-Gagnon and @dzsquared

@erinstellato-ms
Copy link

erinstellato-ms commented Feb 17, 2023

We can either put these schemas in their own folder, or hide them altogether. It's on my list to collect feedback about it from users, but official guidance from MS says to not put any objects in these schemas (https://learn.microsoft.com/en-us/sql/relational-databases/security/authentication-access/ownership-and-user-schema-separation?view=sql-server-ver16#built-in-schemas-for-backward-compatibility). In fact, I've already had a MS PM message me about showing these schemas (and says we should not).

A reasonable compromise, to start, is to have them in their own folder. Then we'll see what feedback we get. I think either Legacy Schemas or Built-In Schemas is fine (I prefer legacy, but I completely get @dzsquared's point about the documentation using Built-It).

The dbo schema should NOT be in there. It is the default schema where objects are created and must be visible.

@dzsquared
Copy link

We can either put these schemas in their own folder, or hide them altogether. It's on my list to collect feedback about it from users, but official guidance from MS says to not put any objects in these schemas (https://learn.microsoft.com/en-us/sql/relational-databases/security/authentication-access/ownership-and-user-schema-separation?view=sql-server-ver16#built-in-schemas-for-backward-compatibility). In fact, I've already had a MS PM message me about showing these schemas (and says we should not).

A reasonable compromise, to start, is to have them in their own folder. Then we'll see what feedback we get. I think either Legacy Schemas or Built-In Schemas is fine (I prefer legacy, but I completely get @dzsquared's point about the documentation using Built-It).

The dbo schema should NOT be in there. It is the default schema where objects are created and must be visible.

if @erinstellato-ms says Legacy, then we go with Legacy. "Not recommended for your health and wellbeing" doesn't reasonably fit.

@erinstellato-ms
Copy link

@dzsquared you're funny :) I'm really good with either legacy or built-in. Both are appropriate (and yes, something like "do not use these" is too long!)

Copy link
Member

@kburtram kburtram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Agree there is probably a better name than legacy, but the change its self is fine.

@aasimkhan30 aasimkhan30 merged commit e8d24f8 into main Feb 17, 2023
@aasimkhan30 aasimkhan30 deleted the aasim/feat/legacySchemas branch February 17, 2023 22:31
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.

7 participants