-
Notifications
You must be signed in to change notification settings - Fork 8
Hi, please consider these changes. #5
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
base: master
Are you sure you want to change the base?
Conversation
… an additional parameter was added to control which type of execution you want to perform. Added logic to check SQL module availability (SqlServer or SQLPS, depending on the version of PowerShell) to speed up execution where this is already available.
|
Hi @glomaga ! Thank you so much for your contribution and sorry for the delay. For some reason, GitHub did not notified me about your PR. I'll make sure to review and test your changes and go forward with your PR. Cheers |
| [string]$openCoverXmlFile, | ||
| [string]$coberturaFileName, | ||
| [string]$htmlReportsOutput, | ||
| [string]$queryTimeout |
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.
There may have a comma missing here, after $queryTimeout.
I think it breaks the code execution. But in case we're not sure, it would be good to keep the standard.
| "name": "enableAzure", | ||
| "type": "boolean", | ||
| "label": "Enable Azure SQL", | ||
| "defaultValue": "false", |
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.
As we've been working on a cloud world, do you think that it makes sense to leave it true by default?
| "label": "Enable Azure SQL", | ||
| "defaultValue": "false", | ||
| "required": false, | ||
| "helpMarkDown": "If you want to get the result from a Azure SQL Server, enable 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.
I was also thinking about requesting this info from the user as a Dropdown list. What do you think?
This way, we could ask for something like:
"SQL Server technology/service type":
- SQL Server On Premises
- Azure SQL
And then we'd handle this on the scripts.
riserrad
left a comment
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.
Such a great idea to support both SQL onprem and Azure SQL by adding some fine tune to it. I loved it.
I left some comments that I'd love to hear from you what you think.
Thanks!
To be able to use the task in a SQL Server environment without Azure, an additional parameter was added to control which type of execution you want to perform.
Added logic to check SQL module availability (SqlServer or SQLPS, depending on the version of PowerShell) to speed up execution where this is already available.