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

Include object types getting from DacFx enum #1553

Closed
wants to merge 3 commits into from

Conversation

ssreerama
Copy link
Contributor

These changes will avoid the duplication of options on extensions by simply getting the name from the DacFx enum.

@ssreerama ssreerama requested a review from kisantia June 21, 2022 20:10
@ssreerama ssreerama marked this pull request as ready for review June 21, 2022 20:10
/// Gets include objects enum names and values as a dictionary
/// Ex: {key: Aggregate, value: 0}
/// </summary>
public void GetIncludeObjectTypesDictionary()
Copy link
Contributor

Choose a reason for hiding this comment

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

Methods with names starting with Get should generally be returning something - would suggest PopulateIncludeObjectTypesDictionary instead

// Verify the include objects are not null
foreach (var includeObjType in options.IncludeObjects)
{
// Verify random include object types
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Why wouldn't you just check the types directly (Assert.AreEqual(0, options.IncludeObjects["Aggregates"].Value))? Looping over the objects isn't really doing anything useful
  2. You should use NUnit fluent assertions whenever possible (Assert.That(includeObjType.Value, Is.EqualTo(0))). It's a lot more flexible, easier to work with and gives better error messages
  3. If an assertion fails it needs to be obvious what's wrong from the log itself, in this case the message will just be something like 0 does not equal 5 which isn't helpful for debugging. In cases like this you should add a further message to indicate the specific problem `Assert.That(options.IncludeObjects["Aggregates"], Is.EqualTo(0), "Aggregates key didn't have expected value")

Although I question the usefulness of this test to begin with - what's it actually verifying? If you just want to make sure the default options are populated then you could simply check the length or something - or if you wanted to check the values might as well check every item (the value should be the value of the enum)

SetOptions(options);
}

/// <summary>
/// Gets include objects enum names and values as a dictionary
/// Ex: {key: Aggregate, value: 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

like I mentioned in the other PR, does ADS need to know about the enum number of each object type? We should only need the name in ADS. If it's needed in in STS for converting before calling the DacServices apis, then a name to enum dictionary can be in STS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for the enum numbers are these selected include objects are being send back to STS with the excludeObjectType property which holds the number of enum values. We can convert it to send back as values instead but there needs to be a conversion back to numbers when sending to DacFx. Let me know if you want me to do the conversion on STS and just sending enum values to the ADS. Thanks :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that the other options have to be converted anyway from the map to DacDeployOptions when coming back from ADS, it would make sense to only send the Schema Object names to ADS, then also do the conversion back to the enum values in STS

Copy link
Contributor

Choose a reason for hiding this comment

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

what about the display names of these objects? Multiple word objects have a different display name from their enum name, like "StoredProcedure" has a space in the display name "Stored Procedure."

Right now, the display names are specified in the schema compare extension. Sql database projects will also need these, so it would be nice if the display names are also coming from STS so they aren't copied in 2 extensions.

{
// Set include objects table data
var objectTypeEnum = typeof(ObjectType);
IncludeObjects = Enum.GetNames(objectTypeEnum).ToDictionary(t => t, t => (int)System.Enum.Parse(objectTypeEnum, t));
Copy link
Member

Choose a reason for hiding this comment

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

using enum number is a bad idea. this means you cannot change the order of values in Enum in dacfx otherwise it can break ADs if the number is used in any extension. don't give any information you don't want any extension to use

@ssreerama
Copy link
Contributor Author

This PR was spliced into two PRs and one was merged into main

  1. Boolean Option types - (Merged) OptionsMapTable preparing from DacFx description,value,displayname at… #1550
  2. Include Object Types - (Open) Object Types are getting from DacFx ObjectTypes #1574

@ssreerama ssreerama closed this Aug 9, 2022
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.

5 participants