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

MySQL storage backend #924

Closed
wants to merge 7 commits into from
Closed

MySQL storage backend #924

wants to merge 7 commits into from

Conversation

pborzenkov
Copy link
Contributor

The pull request adds MySQL storage backend.

It will be shared by both Postgres and MySQL configs.

Signed-off-by: Pavel Borzenkov <[email protected]>
It will be used by MySQL test function as well.

Signed-off-by: Pavel Borzenkov <[email protected]>
MySQL driver doesn't support this.

Signed-off-by: Pavel Borzenkov <[email protected]>
There is no way to configure SSL yet.

Signed-off-by: Pavel Borzenkov <[email protected]>
@pborzenkov
Copy link
Contributor Author

Hmm, looks like Tarvis is using MySQL 5.5 which doesn't support fractional DATETIME values. 5.6 is available in 'trusty' CI environment. I'll remove MySQL backend from CI until the switch is made.

Copy link
Contributor

@rithujohn191 rithujohn191 left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. Overall looks good. But we will have to ensure that the Travis tests pass before integrating. We don't plan to switch over to 'trusty' any time soon so will it be possible for you to make do with MySQL 5.5 and eliminate the need for supporting fractional DATETIME values?

)

// NetworkDB contains options common to SQL databases accessed over network.
type NetworkDB struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add individual comments describing each attribute.

Timeout: time.Second * time.Duration(s.ConnectionTimeout),

ParseTime: true,
Params: map[string]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment to explain this

@pborzenkov
Copy link
Contributor Author

@rithujohn191 The problem is that conformance tests require the storage to provide millisecond precision right now. I see several ways to fix this:

  • Keep the precision requirement and require MySQL 5.6. This way either CI has to be switched to 'trusty' or MySQL >= 5.6 has to be started in a docker container;
  • Relax the precision requirement in the tests and agree to full second precision;
  • Extend the Storage interface with the 'TimestampPrecision() time.Duration' method which will be used to report the maximum timestamp precision supported by the storage. And use the method throughout the test code.

Personally, I think the third one is the way to go and it does not require a lot of coding. What do you think?

@michelaquino
Copy link

Hi guys, what the PR status?

@pborzenkov
Copy link
Contributor Author

Unfortunately, I didn't have time to finish it. I'll try to complete it in the coming days. I'll go with the item 2 from my proposal (relaxing timestamp precision requirements to full second).

@ericchiang
Copy link
Contributor

I don't think the dex team has the bandwidth to maintain another storage implementation. If someone wants to step up as a maintainer, we would consider merging, But without someone dedicated to this storage, I don't think this is going to happen today.

@sagikazarmark
Copy link
Member

Dex supports MySQL now.

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.

5 participants