Skip to content

MySQL physical backend implementation#324

Merged
armon merged 5 commits intohashicorp:masterfrom
pradeepchhetri:master
Jun 18, 2015
Merged

MySQL physical backend implementation#324
armon merged 5 commits intohashicorp:masterfrom
pradeepchhetri:master

Conversation

@pradeepchhetri
Copy link
Copy Markdown
Contributor

I am new to Golang. I followed the logic in existing backends and tried to port it for MySQL. I am not sure if having MySQL as one of the physical makes sense or not. Kindly please review the code and provide me feedback.

Thank you !!

Comment thread physical/mysql.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You probably only need to put the errors that are used more than once at this level. The single use errors can just be defined at the call site.

@armon
Copy link
Copy Markdown
Contributor

armon commented Jun 11, 2015

Left some comments, but this is looking on the right track!

@pradeepchhetri
Copy link
Copy Markdown
Contributor Author

Hello @armon !! I updated things as per your suggestions except following things:

  • I wasn't able to make create table statement as constant since it is having database and table name which is coming from config.
  • I wasn't able to build vault from source. It fails with msgp command not found. I wasn't able to figure out that from which package does msgp binary comes from.

Can you please look into the code once again and see if it looks as per your comments.

@pradeepchhetri
Copy link
Copy Markdown
Contributor Author

@armon I was able to build and run the tests. One test which is trying to select the value for key when the key doesn't exists is still failing otherwise other tests are working fine. Can you suggest me how should i solve that ?

@pradeepchhetri
Copy link
Copy Markdown
Contributor Author

Fixed the failing test. Can you please review once again.

Comment thread physical/mysql.go
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks suspect, I think we need to properly use the mysql escaping to avoid an issue with prefix being an attack vector

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@armon I am unable to figure out how to do mysql escaping. Can you provide me link of some example. Sorry to bother you.

@armon
Copy link
Copy Markdown
Contributor

armon commented Jun 17, 2015

@pradeepchhetri Looking good, just one minor issue with List

@armon armon merged commit 7c7f64f into hashicorp:master Jun 18, 2015
@armon
Copy link
Copy Markdown
Contributor

armon commented Jun 18, 2015

@pradeepchhetri I merged this and made some minor cleanups. Thanks!

@pradeepchhetri
Copy link
Copy Markdown
Contributor Author

@armon Thank you for all the feedbacks. Learnt a lot :)

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.

2 participants