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

fix(export): Escape MySQL column names #8947

Closed
wants to merge 3 commits into from

Conversation

andres-lowrie
Copy link
Contributor

Description: This change wraps all the column names with backticks so that MySQL won't complain when a table has columns that are also reserved keywords in MySQL.

@CLAassistant
Copy link

CLAassistant commented Aug 11, 2023

CLA assistant check
All committers have signed the CLA.

@mangalaman93
Copy link
Contributor

Thanks for updating the PR. Please create the PR against the main branch and sign the CLA. You will see a button above. Thank you.

Copy link
Contributor

@darkn3rd darkn3rd left a comment

Choose a reason for hiding this comment

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

LGTM

@mangalaman93
Copy link
Contributor

@andres-lowrie thanks for signing the CLA. The linter is not happy, could you take a look? Happy to help if it is not straight forward for you to figure out.

@joshua-goldstein
Copy link
Contributor

@andres-lowrie Thanks for the PR - I think this PR needs to be rebased (or recreated) against our main branch. Check that you have our latest main and that this PR is branched off from there. I see that the base branch was changed from master to main but the original commits were presumably against master. This should resolve the CI issues. I would do this myself but since this is on your fork it is not possible for me to do.
cc' @mangalaman93

@mangalaman93
Copy link
Contributor

Maybe the linter issue is also because this PR does not have latest main.

@joshua-goldstein
Copy link
Contributor

Maybe the linter issue is also because this PR does not have latest main.

Correct - the jobs that are running (and failing) are coming from workflow files checked into the old master branch. The jobs marked as required are coming from our PR merge requirements on github. Hence the confusion in CI. They are not running because the workflow files are presumably not in the the branch andres-lowrie:master.

Fortunately this PR has a relatively small surface area, so in the worst case @andres-lowrie can recreate it relatively easily (from our main branch).

@andres-lowrie
Copy link
Contributor Author

oops yeah I think I get it now. I'll re-do the changes from main after updating my fork

@andres-lowrie
Copy link
Contributor Author

let me close this one out

@andres-lowrie
Copy link
Contributor Author

created other pr #8961

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants