-
Notifications
You must be signed in to change notification settings - Fork 9
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
OSOE-481: Action pulls latest SQL Server (=2022-latest) but 2019-latest is used in Lombiq.GitHub.Actions #293
Changes from 7 commits
0a1f2bd
56381dc
9518cc6
ec6e6fd
7cecb90
76f1c96
0e895ef
75a2b21
0ecf4cd
ea2c39b
a5e354a
9c5553e
b313016
d481e0e
04cb286
31a9026
7ba9378
2e8bddf
1085d06
c3138ff
941f172
a06d50c
0bc635f
ba19eaa
d03e6d4
53a6d0e
1ab8ff7
6eaae21
4c3861f
44f0be8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,20 @@ | ||
if ($Env:RUNNER_OS -eq 'Windows') | ||
{ | ||
choco install sql-server-express --no-progress | ||
choco install sql-server-2022 --no-progress | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DRY There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was not addressed. |
||
} | ||
else | ||
{ | ||
$sqlServerName = 'sql2022' | ||
$sqlServerVersion = '2022-latest' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use only the version eg Why don't you want to create a script parameter for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we are making a load of changes to a script that was working fine and nobody wanted to touch it, until we realised that 2019 should be swapped to 2022. So according to clean code yes your recommendations are valid but the use case here is so minor and easy I think we are overcomplicating this at this point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are making changes because there is a bug in our script and we found a maintainability problem while fixing it. Script parameter:
Variable:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did change to param now. |
||
$dockerRunSwitches = @( | ||
'--name', 'sql2019' | ||
'--name', $sqlServerName | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Set the container name here, value 'uitt-sqlserver'. Also see my comment below ( |
||
'--env', 'ACCEPT_EULA=Y' | ||
'--env', 'SA_PASSWORD=Password1!' | ||
'--publish', '1433:1433' | ||
'--detach', 'mcr.microsoft.com/mssql/server:2019-latest' | ||
'--detach', "mcr.microsoft.com/mssql/server:$sqlServerVersion" | ||
) | ||
|
||
docker pull mcr.microsoft.com/mssql/server && | ||
docker pull "mcr.microsoft.com/mssql/server:$sqlServerVersion" && | ||
docker run @dockerRunSwitches && | ||
docker exec --user 0 sql2019 bash -c 'mkdir /data; chmod 777 /data --recursive; chown mssql:root /data' | ||
docker exec --user 0 $sqlServerName bash -c 'mkdir /data; chmod 777 /data --recursive; chown mssql:root /data' | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ else | |
{ | ||
$connectionStringStem = 'Server=.;Database=LombiqUITestingToolbox_{{id}};User Id=sa;Password=Password1!' | ||
|
||
$Env:Lombiq_Tests_UI__DockerConfiguration__ContainerName = 'sql2019' | ||
$Env:Lombiq_Tests_UI__DockerConfiguration__ContainerName = 'sql2022' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can set the container name here. This is the answer to your question (#293 (comment)), value |
||
} | ||
|
||
$Env:Lombiq_Tests_UI__SqlServerDatabaseConfiguration__ConnectionStringTemplate = $connectionStringStem + $connectionStringSuffix | ||
|
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.
In line 3, please do what @Piedone asked here.
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.
Thats what I asked for in my previous comment whether it is acceptable that we use SQL SERVER DEVELOPER instead of EXPRESS becasue I didn't find any particular command in the documentation that specifies for Express 2022.
So basically
sql-server-express
is dedicated for 2022 express and I didn't find any other command for it.sql-server-2022
will install Developer.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.
Developer would actually be better.
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 changed
sql-server-express
tosql-server-2022
which eventually will install the Developer versionThere 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.
Also it makes sure that it's version 2022 and its fixed.