Generating Default Config using CLI #930

severussundar · 2022-10-31T11:56:12Z

What is this change?

A file containing the commands to generate the default config file dab-config.json is added
The scripts are updated to read and execute commands from this newly added file.
The contents of the ConfigGenerators are added to the sln file. Now, the command files and scripts show up in Visual Studio. Earlier, they were not, and the commands/scripts had to be edited using another text editor/IDE.

How was this tested?

Existing Integration Tests and Unit Tests run successfully

…erences

…easy editing using visual studio

seantleonard

Some questions/concerns

ConfigGenerators/configGenerator.ps1

ConfigGenerators/configGenerator.sh

src/Azure.DataApiBuilder.sln

.pipelines/build-pipelines.yml

ConfigGenerators/configGenerator.sh

Aniruddh25

Need a reason to generate the default config

ConfigGenerators/DefaultConfigCommands.txt

…configuration

…on' of https://github.com/Azure/data-api-builder into dev/shyamsundarj/do-not-copy-configs-docker-configuration

seantleonard · 2022-11-02T17:00:46Z

src/Azure.DataApiBuilder.sln

+		..\ConfigGenerators\configGenerator.ps1 = ..\ConfigGenerators\configGenerator.ps1
+		..\ConfigGenerators\configGenerator.sh = ..\ConfigGenerators\configGenerator.sh


Is Visual Studio smart enough to know which of these to run per platform? or do both attempt to run?

Oh, are these just file references for what is in the solution? I'm not recognizing why these lines are added to this file/what the significance is.

seantleonard · 2022-11-02T17:02:37Z

src/Cli/test/EndToEndTests.cs

-    // Test to verify the engine gets started using start command
-    // </summary>
+    /// <summary>
+    /// Test to verify the engine starts with the right log level using start command


Suggested change

/// Test to verify the engine starts with the right log level using start command

/// Test to verify the engine starts with the log level specified in the start command

This is what is meant, yes?

Exactly! Will update

seantleonard

Some more questions

seantleonard · 2022-11-02T17:05:04Z

src/Cli/test/EndToEndTests.cs

    [DataRow("--LogLevel CrItIcal", false, true, DisplayName = "Case sensitivity: LogLevel Critical from command line.")]
    [DataRow("--LogLevel NONE", false, true, DisplayName = "Case sensitivity: LogLevel None from command line.")]
-    public void TestStartEngine(string logging, bool useDefaultConfig, bool expectSuccess)
+    public void TestStartEngine(string logging, bool useInvalidConfig, bool expectSuccess)


Can you add parameter comments to the test summary?

Define how useInvalidConfig is relevant to testing the logging level detected during startup

seantleonard · 2022-11-02T17:14:55Z

src/Cli/test/TestHelper.cs

        public static string _testRuntimeConfig = "dab-config-test.json";

+        // Name of an invalid config file that can be used for tests.
+        public static string InvalidConfigFileName


Elaborate in comment what you are trying to achieve with an invalid config file name.

Are you trying to get the engine to start without a config file and if so, could the same behavior be achieved by using option --config with no value?

seantleonard · 2022-11-02T17:17:23Z

src/Cli/test/TestHelper.cs

+            get
+            {
+                return "dab-config.invalid.json";
+            }


any reason you chose property/getter vs. constant?

SO discussion ( just for consideration, I'm not suggesting one implementation vs. another)

https://stackoverflow.com/a/10362203/18174950

https://stackoverflow.com/a/755693/18174950

seantleonard · 2022-11-02T17:19:06Z

src/Cli/test/TestHelper.cs

        public const string CONFIG_WITH_SINGLE_ENTITY = @"
          {
-        ""$schema"": ""dab.draft-01.schema.json"",
+        ""$schema"": ""dab.draft.schema.json"",


Could you use the const that you defined elsewhere in this PR: SCHEMA ?

seantleonard · 2022-11-02T17:20:55Z

src/Service.Tests/Unittests/RestServiceUnitTests.cs

    {
        private static RestService _restService;
-        private static string _testCategory = "mssql";
+        private static string _testCategory = "MsSql";


Is this variable necessary? could you use TestCategory.MSSQL instead of _testCategory ?

seantleonard · 2022-11-02T17:23:29Z

src/Azure.DataApiBuilder.sln

+		..\ConfigGenerators\configGenerator.ps1 = ..\ConfigGenerators\configGenerator.ps1
+		..\ConfigGenerators\configGenerator.sh = ..\ConfigGenerators\configGenerator.sh


Oh, are these just file references for what is in the solution? I'm not recognizing why these lines are added to this file/what the significance is.

severussundar · 2022-11-17T12:56:12Z

Closing this as PR 981 takes care of the changes present in this one

severussundar added 4 commits October 30, 2022 17:20

removing --no-build argument when creating nuget, removing config ref…

82145bf

…erences

Generating default config using cli, adding command files to sln for …

1cdde36

…easy editing using visual studio

removing checked-in default config

328bf89

removing config files itemgroup

0ef4d69

severussundar requested review from Aniruddh25, JelteF, Mathos1432, aaronburtle, aaronpowell, abhishekkumams, ayush3797, gledis69, jarupatj, junsu0ms, mbhaskar, milismsft, seantleonard and tarazou9 as code owners October 31, 2022 11:56

severussundar self-assigned this Oct 31, 2022

severussundar added this to the Oct2022 milestone Oct 31, 2022

severussundar added the config changes related to config label Oct 31, 2022

removing dab-config.json from sln

d78b500

seantleonard requested changes Oct 31, 2022

View reviewed changes

ConfigGenerators/configGenerator.ps1 Outdated Show resolved Hide resolved

ConfigGenerators/configGenerator.sh Outdated Show resolved Hide resolved

src/Azure.DataApiBuilder.sln Show resolved Hide resolved

.pipelines/build-pipelines.yml Outdated Show resolved Hide resolved

Aniruddh25 reviewed Oct 31, 2022

View reviewed changes

ConfigGenerators/configGenerator.sh Outdated Show resolved Hide resolved

Aniruddh25 requested changes Oct 31, 2022

View reviewed changes

aaronburtle reviewed Nov 1, 2022

View reviewed changes

ConfigGenerators/DefaultConfigCommands.txt Outdated Show resolved Hide resolved

severussundar modified the milestones: Oct2022, Nov2022 Nov 1, 2022

severussundar added 2 commits November 2, 2022 12:06

updating unit tests that uses dab-config.json file

10921b4

Merge branch 'main' into dev/shyamsundarj/do-not-copy-configs-docker-…

7434b92

…configuration

severussundar added 5 commits November 2, 2022 12:09

re-writing a condition

b21d9c0

Merge branch 'dev/shyamsundarj/do-not-copy-configs-docker-configurati…

fc9f67f

…on' of https://github.com/Azure/data-api-builder into dev/shyamsundarj/do-not-copy-configs-docker-configuration

changing the case of MsSql

8b0839b

moving invalid config name to testhelper

ad8e3a4

fix formatting

9e7b5ba

seantleonard reviewed Nov 2, 2022

View reviewed changes

seantleonard requested changes Nov 2, 2022

View reviewed changes

Aniruddh25 approved these changes Nov 3, 2022

View reviewed changes

severussundar closed this Nov 17, 2022

		..\ConfigGenerators\configGenerator.ps1 = ..\ConfigGenerators\configGenerator.ps1
		..\ConfigGenerators\configGenerator.sh = ..\ConfigGenerators\configGenerator.sh

	/// Test to verify the engine starts with the right log level using start command
	/// Test to verify the engine starts with the log level specified in the start command

Generating Default Config using CLI #930

Generating Default Config using CLI #930

Uh oh!

Conversation

severussundar commented Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this change?

How was this tested?

Uh oh!

seantleonard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aniruddh25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seantleonard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

severussundar commented Nov 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

severussundar commented Oct 31, 2022 •

edited

Loading