-
Notifications
You must be signed in to change notification settings - Fork 108
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
Project improvements #813
Project improvements #813
Conversation
Tests were not changed in this PR, so some of them will get errors, as it happened before. I think that is why tests CI job doesn't pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this cleanup looks good. once it merges I think i can disable circleCI for the repo
I'm looking at the status checks on this PR to switch over the required ones for merges to master, and I don't see the new ones as being run. Is this because it's coming from a fork, or is it unexpected we wouldn't see the |
@@ -38,7 +38,7 @@ func New(address, certPath string, insecure bool) (client.Client, error) { | |||
} else if insecure { | |||
opts = append(opts, grpc.WithInsecure()) | |||
} else { | |||
opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{}))) | |||
opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12}))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@willscott Just flagging that we're putting a minimum now on our TLS connection - I think we should be fine but we should make sure all our partners have TLS1.2 at the very least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to add this to the README. This was something linter required
Oh right yes, sorry, well I guess we just need to make sure the `make
install` still works properly that's what is in the main README
Le lun. 27 sept. 2021 à 14:03, Emmanuel ***@***.***> a écrit :
… ***@***.**** commented on this pull request.
------------------------------
In docker/README.md
<#813 (comment)>:
> @@ -0,0 +1,6 @@
+## Instructions
But this README is meant to help with Docker images! I can add them on the
general README if you want to
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#813 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AATSFC7PRBS27GOA7DO5REDUEBTRDANCNFSM5ESAQEEQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
--
Nicolas Gailly
|
That command is still working! No problem at all. |
I added PRs to the events that run that job (build/tests). It should run now. |
In the PR many improvements where applied. A list of them is write below: