Skip to content

Better support comments in mysql files#54371

Merged
dwoz merged 11 commits intosaltstack:masterfrom
mchugh19:47488
Apr 13, 2020
Merged

Better support comments in mysql files#54371
dwoz merged 11 commits intosaltstack:masterfrom
mchugh19:47488

Conversation

@mchugh19
Copy link
Contributor

What does this PR do?

Better handles comments in mysql files.
Also adds a no_parse flag to pass the entirety of the mysql file contents to mysql. This allows mysql to handle its own parsing, which might work better for some complex setups, but also prevents salt from returning specifics of what has changed.

What issues does this PR fix or reference?

#47488

Previous Behavior

A file like

-- some comment
INSERT INTO users (username) VALUES ("sampleuser2");  # after
#another comment
INSERT INTO users (username) VALUES ("sampleuser3"); 

would be interpreted as

INSERT INTO users (username) VALUES ("sampleuser2");
# after #another comment INSERT INTO users (username) VALUES ("sampleuser3");

New Behavior

Now that file is interpreted as

INSERT INTO users (username) VALUES ("sampleuser2");
INSERT INTO users (username) VALUES ("sampleuser3");

When no_parse is used, the execution module will return something like the following for files which contain many create, insert, and update statements.

local:
    ----------
    query time:
        ----------
        human:
            0.0s
        raw:
            0.00042
    rows affected:
        1  

Tests written?

Yes - existing test updated to support this functionality

Commits signed with GPG?

No

@mchugh19 mchugh19 requested a review from a team as a code owner August 31, 2019 10:28
@ghost ghost requested a review from DmitryKuzmenko August 31, 2019 10:28
@mchugh19 mchugh19 changed the base branch from develop to neon November 17, 2019 18:04
@mchugh19 mchugh19 changed the base branch from neon to master December 1, 2019 15:41
@mchugh19
Copy link
Contributor Author

mchugh19 commented Dec 1, 2019

Rebased on master

@DmitryKuzmenko
Copy link
Contributor

@mchugh19 thank you for this work! It's awesome!
It's not a full support of MySQL/MariaDB comments style, but it's much better than it was.
The only thing I want to ask you. You're using regex that is as powerful as hardly readable and supportable. Meanwhile our tests don't cover your functionality. You've added a generic regression update to an existing integration test. But we definitely need tests:

  1. checking your regular expressions in all combinations of data;
  2. covering the no_parse case.

There's already a unit test for mysql module so it'd be pretty simple to extend this.
Are you able to extend the test to your functionality please?

@DmitryKuzmenko
Copy link
Contributor

Additional thanks for blacken your code!

@DmitryKuzmenko DmitryKuzmenko self-assigned this Apr 8, 2020
@mchugh19
Copy link
Contributor Author

mchugh19 commented Apr 8, 2020

Never say nothing good came out of the test cases! Since getting back to this, I was not able to get the no_parse feature to work with these libraries. Since it's that flakey, I just removed the whole thing. An additional unit test was written for the comment handling.

Copy link
Contributor

@DmitryKuzmenko DmitryKuzmenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please understand me correctly I'm not trying to bore you. =) Thanks for understanding!

@DmitryKuzmenko
Copy link
Contributor

DmitryKuzmenko commented Apr 9, 2020

This PR is blackened and merge ready.

@dwoz dwoz merged commit 1796328 into saltstack:master Apr 13, 2020
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ZRelease-Sodium retired label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants