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

initial pass at adding tls support to mysql #82

Merged
merged 8 commits into from
May 19, 2022

Conversation

MattMencel
Copy link
Contributor

@MattMencel MattMencel commented Apr 7, 2022

Description of your changes

Enables tls for MySQL connections. Sets preferred as the default which will fall back to non-tls if unsupported by the MySQL instance.

"Fixes #70":
"Fixes #81":

I have:

  • Read and followed Crossplane's [contribution process].
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

make reviewable

Copy link
Member

@Duologic Duologic left a comment

Choose a reason for hiding this comment

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

The tests seem to fail, can you have a look? Note that the golangci-lint version might differ:

GO_VERSION: '1.17'
GOLANGCI_VERSION: 'v1.31'
DOCKER_BUILDX_VERSION: 'v0.7.1'

pkg/clients/mysql/mysql.go Show resolved Hide resolved
@MattMencel
Copy link
Contributor Author

The tests seem to fail, can you have a look? Note that the golangci-lint version might differ:

GO_VERSION: '1.17'
GOLANGCI_VERSION: 'v1.31'
DOCKER_BUILDX_VERSION: 'v0.7.1'

I'm trying to get these tests to run in my fork's Github Actions so I can debug further. Same as Alex in #84 mentions, I'm not sure what to do to test locally. Maybe a quick guide on how to run them locally?

@Duologic
Copy link
Member

Duologic commented Apr 11, 2022

make reviewable fails when I check out your branch, following make targets should change files that fix the CI:

make fmt.simplify
make generate

@MattMencel
Copy link
Contributor Author

MattMencel commented Apr 12, 2022

Both those commands result in this...

> make fmt.simplify
make: *** No rule to make target `fmt.simplify'.  Stop.

I figured it out I think. Added some docs to the README to help future contributors who might not know what they are doing, like me. 😁

@MattMencel
Copy link
Contributor Author

I bumped golangci-lint to 1.45 in the Github Action. It doesn't seem like there's a simple way to downgrade it on a Mac with Homebrew. All the tests are passing in my fork now.

Also, the CI pipeline has golang 1.17 pinned, but the go.mod file is still at 1.13?

@MattMencel MattMencel marked this pull request as ready for review April 12, 2022 15:00
@MattMencel MattMencel requested a review from Duologic April 12, 2022 15:00
@Duologic
Copy link
Member

I had a look at bumping the golangci-lint before but it is also set in the build tools. I think it is best to keep those aligned. https://github.com/upbound/build/blob/master/makelib/golang.mk#L99

If we want to bump versions, I'd suggest doing that in a separate PR so we can also adress the versions in the upstream repo.

@MattMencel
Copy link
Contributor Author

@Duologic I reverted the golangci-lint change

@MattMencel MattMencel force-pushed the mysql-tls branch 2 times, most recently from 1ba9d0e to ffcd036 Compare April 13, 2022 18:50
@Duologic
Copy link
Member

I'm attempting to fix the lint CI job in #88

@Duologic Duologic mentioned this pull request Apr 29, 2022
@Duologic
Copy link
Member

#88 is merged, do you mind rebasing?

@MattMencel
Copy link
Contributor Author

@Duologic all the tests passed in my fork

@MattMencel MattMencel force-pushed the mysql-tls branch 2 times, most recently from 5378a98 to df49141 Compare May 5, 2022 02:08
@Duologic
Copy link
Member

Duologic commented May 6, 2022

I think we need to revert the change on the build submodule and follow master.

@MattMencel
Copy link
Contributor Author

I think we need to revert the change on the build submodule and follow master.

Yeah I tried to debug it yesterday but I wasn't getting anywhere. So change *string back to string you're saying?

@Duologic Duologic mentioned this pull request May 11, 2022
@Duologic
Copy link
Member

@MattMencel I managed to get it working in #91, you can cherry-pick my commits if you like.

MattMencel and others added 5 commits May 15, 2022 22:33
@MattMencel
Copy link
Contributor Author

@MattMencel I managed to get it working in #91, you can cherry-pick my commits if you like.

@Duologic cherry-picked

@Duologic Duologic merged commit ed6c55c into crossplane-contrib:master May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants