chore(controller-runtime) : Update controller-runtime version #4227
Conversation
snehachhabria
force-pushed
the
setTlsMinimum
branch
3 times, most recently
from
48462b7
to
ae0766f
Compare
| @@ -74,6 +74,10 @@ jobs: | |||
| steps: | |||
| - name: Checkout | |||
| uses: actions/checkout@v1 | |||
| - name: Setup Go 1.16 | |||
There was a problem hiding this comment.
runners don't have go preinstalled (according to this list) and we never have the "Setup Go" step this job. Not sure how it was running and validating all along
There was a problem hiding this comment.
Let's investigate why that's passing, and if this is even required. It also seems totally unrelated to this PR.
There was a problem hiding this comment.
The mock check for this PR will not pass without this, so it can be very much a related fix to this PR
Yes it is, please refer to this issue for more details : #4165 |
|
@shashankram from #4165 that CR needs to be 0.9.1+ to support configuring minimum TLS version. @snehachhabria is there a reason we are defaulting to TLS 1.2 instead of TLS 1.3? Is it because of the Kubernetes support for TLS 1.3 requiring 1.17? |
We do not use controller-runtime for TLS, it's just used for creating JSON patches and hence there should be no dependency on updating controller-runtime in a PR that suggests the TLS version is being fixed. Please make the controller-runtime change in a separate PR if necessary, as I do not believe it is related to TLS in any way. |
snehachhabria
changed the title
snehachhabria
force-pushed
the
setTlsMinimum
branch
from
ae0766f
to
1d2e903
Compare
Update controller-runtime to v0.10.2 part of openservicemesh#4165 Signed-off-by: Sneha Chhabria <snchh@microsoft.com>
There was a problem hiding this comment.
I'd like to see this PR split up and scoped to the TLS version updates.
I don't believe controller-runtime needs to be updated, even though the issue suggests to update it, because we do not use controller-runtime other than for patching JSON.
Could you split this up and isolate this change to be specific to TLS config updates? Else it's just confusing to review.
| @@ -74,6 +74,10 @@ jobs: | |||
| steps: | |||
| - name: Checkout | |||
| uses: actions/checkout@v1 | |||
| - name: Setup Go 1.16 | |||
There was a problem hiding this comment.
Let's investigate why that's passing, and if this is even required. It also seems totally unrelated to this PR.
I've updated this PR to just update the controller runtime, I will send the TLS min version in a follow up PR |
Since OSM is supported for k8s version 1.18 and above I guess we can bump this up to 1.3. I'll send the changes in a separate PR |
|
synced with @shashankram offline and we will skip updating the controller-runtime version for now. I'll send a separate PR for the TLS min version |
Description:
Update controller-runtime to v0.10.2
part of #4165
Signed-off-by: Sneha Chhabria snchh@microsoft.com
Affected area:
Please answer the following questions with yes/no.
Does this change contain code from or inspired by another project?
noIs this a breaking change?
no