Azure / sql-action

🚀 Deploy changes to your SQL database easily with SQL projects or SQL scripts and sql-action for GitHub workflows
MIT License
103 stars 58 forks source link

Breaking change to arguments in v2.3 #235

Closed emil-eklund closed 4 months ago

emil-eklund commented 4 months ago

We are experiencing some issues since the latest release (v2.3). I don't know if this is a bug or intentional, but it seems as if parameters passed are forwarded incorrectly.

image

Logs

This is the pipeline step which has started to fail:

    - name: Alter PBI user with new password
      uses: Azure/sql-action@master
      with:
        connection-string: >-
          Data Source=${{ vars.SERVER_NAME }}.database.windows.net;
          Initial Catalog=I${{ env.padded_id }};
          Authentication=Active Directory Default;
          Encrypt=True;
          TrustServerCertificate=False;
        path: alter-pbi-user.sql
        arguments: >
          -V11 
          -b 
          -v PaddedId="${{ env.padded_id }}"
          -v PowerBiPassword="${{ env.power_bi_password }}"

I will investigate this further soon, but maybe you already know what's changed.

zijchen commented 4 months ago

Hi @emil-eklund What version of sql-action were you using prior to v2.3?

dzsquared commented 4 months ago

-V11 is not a valid argument for go-sqlcmd

nor was it documented for the ODBC sqlcmd, so I'm not positive of its behavior - https://learn.microsoft.com/en-us/sql/tools/sqlcmd/sqlcmd-utility?view=sql-server-ver16&tabs=odbc%2Cwindows&pivots=cs1-bash

emil-eklund commented 4 months ago

Hi,

@zijchen Seems like we were using @master, so I suspect it's the latest release (v2.2.1).

@dzsquared Are you sure? We've been using it for a long time. I think it sets which severity level is fatal to the command. We ha instances in the past where it would continue executing as if all were well, despite fatal errors.

Use the following practices to help maximize correctness:

Use -V16 to log any severity 16 level messages. Severity 16 messages indicate general errors that can be corrected by the user.

Check the exit code and DOS ERRORLEVEL variable after the process has exited. sqlcmd returns 0 normally, otherwise it sets the ERRORLEVEL as configured by -V. In other words, ERRORLEVEL shouldn't be expected to be the same value as the error number reported from SQL Server. The error number is a SQL Server-specific value corresponding to the system function @@ERROR. ERRORLEVEL is a sqlcmd-specific value to indicate why sqlcmd terminated, and its value is influenced by specifying -b command line argument.

Using -V16 in combination with checking the exit code and DOS ERRORLEVEL can help catch errors in automated environments, particularly quality gates before a production release.

dzsquared commented 4 months ago

shouldn't it be -V 11 instead of -V11

(looks like that example in the docs is completely mangled, that's unfortunate)

emil-eklund commented 4 months ago

shouldn't it be -V 11 instead of -V11

Maybe. It would make a lot more sense, but that's not what the documentation said 💁‍♂️ I'll do some testing tomorrow. If that works, then that would be a great solution!!

There is this curious text in the documentation too:

Currently, sqlcmd doesn't require a space between the command-line option and the value. However, in a future release, a space may be required between the command-line option and the value.

Maybe the "future release" which would break this has arrived!

emil-eklund commented 4 months ago

Thank you @dzsquared & @zijchen for jumping on this so quickly. I tried using "-V 11" as you mentioned, and it is working! 🥳

I'd love to find out why this stopped working and if it's likely to impact anyone else, but as I'm squeezed for time, I'll just close this ticket.

Once again, thank you for helping! You saved me!

emil-eklund commented 4 months ago

Just in case somebody stumbles upon this in the future, we encountered the same issue with another parameter. If you used something like "-h-1" to remove headers, then this breaks in the same manner. Solution is to use "-h -1" instead.

shueybubbles commented 4 months ago

there's a bug in sqlcmd's parsing of arguments that come after -i filename -i can take multiple file names, and the lack of space fools the parser into thinking the argument is a file name instead of an argument.

dzsquared commented 4 months ago

@shueybubbles - did you implement argument parsing without spaces in go-sqlcmd? sql-action uses go-sqlcmd and based on docs I've seen on go-sqlcmd it doesn't support no spaces.