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

linux additional arguments fix #14183

Merged
merged 7 commits into from
Jan 13, 2021
Merged

linux additional arguments fix #14183

merged 7 commits into from
Jan 13, 2021

Conversation

AmrutaKawade
Copy link
Contributor

@AmrutaKawade AmrutaKawade commented Jan 6, 2021

Task name: AzureMysqlDeploymentV1

Description:

Task fails for below combination

  1. Linux Agent + Mysql script file selection + Additional arguments
    Task fails because of not having semi-colon (;) following the MySQL Script file path.

Documentation changes required: (Y/N) N

Added unit tests: (Y/N) N

Attached related issue: (Y/N) #14075

Checklist:

  • Task version was bumped - please check instruction how to do it
  • Checked that applied changes work as expected

@asranja
Copy link
Contributor

asranja commented Jan 6, 2021

Will this be required for MysqlDeploymentOnMachineGroupV1 task as well ?

@AmrutaKawade AmrutaKawade merged commit f462aea into master Jan 13, 2021
@mattboothman
Copy link

This update has broken builds on our Windows build agents

@mattboothman
Copy link

I did a bunch of testing this morning.

The issue is related to the source command itself on Windows and specifically where the full file path is specified, but only where there is a semicolon at the end of the file path.

All of the following tests are on Windows in cmd.exe

Both of these work

source foo.sql
set @foo='bar';source foo.sql

Both of these do not work:

source c:\users\foo\bar\foo.sql;
set @foo='bar';source c:\users\foo\bar\foo.sql;

Both result in the same error locally:

ERROR at line 1: Unknown command '\U'.

I'm not sure why, but the error from the agent is different:

ERROR at line 1: Unknown command '\a'.

MySQL Manual on source client command

The MySQL client manual for commands: https://dev.mysql.com/doc/refman/8.0/en/mysql-commands.html

For the source command:

source file_name, \. file_name

Read the named file and executes the statements contained therein. On Windows, specify path name separators as / or \\.

Quote characters are taken as part of the file name itself. For best results, the name should not include space characters.

Important: I tried switching the path separator \ with \\ and MySQL client emitted a different error:

ERROR at line 1: Unknown command '\\'. 

Before I went searching for the manual, which reads "Quote characters are taken as part of the file name itself" I tried quotes and backticks. So it makes sense adding these didn't work.

The final alternative was to switch out the path separators for / which is the manual's first recommendation.

Finally, both of these work:

source c:/users/foo/bar/foo.sql;
set @foo='bar';source c:/users/foo/bar/foo.sql;

Microsoft docs on AzureMySqlDeployment task

Docs: https://docs.microsoft.com/en-us/azure/devops/pipelines/tasks/deploy/azure-mysql-deployment?view=azure-devops

The documentation for the SqlFile parameter says:

(Required) Full path of the script file on the automation agent or on a UNC path accessible to the automation agent like, \BudgetIT\DeployBuilds\script.sql. Also, predefined system variables like, $(agent.releaseDirectory) can also be used here. A file containing SQL statements can be used here

(note case difference on S in SqlFile between the usage example at the top of the page and the Arguments table. Example is lower, table is upper. Probably makes no difference, but... consistency... our usage is upper)

The docs are wrong in this case so would need updating too. Probably adding a note about the path issue.

Weirdness in version 1.179.0

Looking at the way the -e option was wrapped in double quotes in the previous version makes me wonder if there was some sort of issue being worked around, in the first place.

"-e source c:\foo\bar\foo.sql"

Rather than the updated, I would say, correct version:

-e "source c:\foo\bar\foo.sql;"

I've looked through the changes and cannot work out why the previous version was wrapping the -e option in double quotes.

Changes

  • 4abbef2c It looks like the original change was simple, to just add a semicolon to the end of the source client command.
  • faa86dbd Added a regex replace which would have kept my builds working, I think
  • 7452d2fc Removed the Windows fix :'-( - See here: linux additional arguments fix #14183 (comment)

Shame about commit 7452d2fc, really. But would that have caused issues where \ was escaping a path character?

Conclusion

Change \ path separator to / as recommended in the MySQL manual (linked above). Probably better to use a tool that understands file paths, rather than regex.

Definitely don't try \\ as recommended in the MySQL manual. This doesn't work

Failing that

Update documentation to document the issue and ask that Windows users use hand-crafted relative paths, using / separator. This might be a painful solution when developing pipelines.

@mattboothman
Copy link

Issue reported here: #14352

@mattboothman
Copy link

mattboothman commented Feb 9, 2021

@AmrutaKawade I agree, -e "source c:\foo\bar\foo.sql;" is the correct format.

sqlInline formats as "-e select * from foo;" which sqlFile used to to before the change. I'm really ot sure why that is.

It's quite a confusing problem, please bear with me:

The actual problem, for windows, is the addition of the ; delimiter, which is also correct. It terminates a SQL statement, it's valid SQL, it's right that it should be there. But for some unknown reason, the combination of the use Windows path separators \ and inclusion of the semicolon causes the MySQL client to emit an obscure error message that's difficult to understand and debug.

The fix, on Windows, is to do as the MySQL Client manual asks and switch the path separator for either \\ or /. However, I tried \\ on the commandline, locally, and mysql emitted an error citing the \\ characters as an invalid command. The unix style path separator /, however, does work.

The PR that introduced this issue was raised by someone using this task with an agent running on a Linux platform. Their issue was actually related to the introduction of additional -e options into the command line, essentially adding additional SQL statements that were executed by the MySQL client before the -e "source ..." option's SQL. This introduction of additional statements before the source statement caused the MySQL client to require that the source statement be terminated with a semicolon ;. But, the addition of that semicolon caused the issue we have, now, on Windows build agents, using Windows style path separators.

I've no doubt, without testing, that the omission of the ';' delimiter after the source statement would have been working on Linux until the OP added additional -e options.

It's a very odd problem.

I believe the only fix that is required, to make this problem go away, is that when the task builds the absolute path used to reference the SQL file passed into the sqlFile argument, it uses Unix style path separators /, rather than Windows \, as the MySQL Client manual suggests.

So rather than:

-e "source c:\foo\bar\foo.sql;"

We sould get:

-e "source c:/foo/bar/foo.sql;"

If you wouldn't mind pointing me in the right direction for documentation on developing DevOps pipeline tasks, I'm quite happy to take a bash at it, testing on both Windows and Linux, with and without additional -e options. I can then sumbit a fix in PR.

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.

4 participants