Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Dependency and golang version updates #1270

Merged
merged 2 commits into from
Aug 10, 2020

Conversation

jimmidyson
Copy link
Contributor

@jimmidyson jimmidyson commented Aug 6, 2020

What this PR does / why we need it:
Upgrades to Go module dependencies, especially controller-runtime, and golang version to 1.14.7.

Includes commits from #1269 to fix go mod running in vendor mode - will rebase once that is merged.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Special notes for your reviewer:

@jimmidyson jimmidyson added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 6, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jimmidyson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 6, 2020
Makefile Outdated Show resolved Hide resolved
@RainbowMango
Copy link
Contributor

Hi @jimmidyson , why split to so many commits for a dependency update thing? Can you squash them?

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 7, 2020
@RainbowMango
Copy link
Contributor

How about updating the version to the latest Go 1.14.7?

@jimmidyson
Copy link
Contributor Author

I've squashed dependency commits and upgraded to golang 1.14.7.

/remove-label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot removed the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 7, 2020
@irfanurrehman
Copy link
Contributor

@jimmidyson thanks for doing this. I am however curious about the need for this update?
IMHO a good strategy to update dependencies and/or go version is when the major releases happen, or some functionality change warrants the update.

@jimmidyson
Copy link
Contributor Author

jimmidyson commented Aug 10, 2020

@irfanurrehman The golang update includes security fixes to packages such as crypto/x509 and net/http, that while I haven't reviewed in depth whether we're affected by them, the upgrade doesn't introduce any risk so I don't see why we wouldn't do the update. The update to controller-runtime includes fixes but also metrics for webhooks (important since #1263 migrated to the controller-runtime webhook server).

IMHO a good strategy to update dependencies and/or go version is when the major releases happen, or some functionality change warrants the update.

I agree if we're doing major version upgrades of dependencies, but otherwise updating to minor or patch releases where backwards compatibility applies, I would prefer to upgrade adhoc. That way the project takes advantage of latest bug fixes, security fixes, and improvements in dependencies as we go, while also reducing the need for a bigger PR to do upgrades later (smaller updates are always preferred imo).

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 10, 2020
@hectorj2f
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2020
@hectorj2f
Copy link
Contributor

This PR just bumps certain dep versions.

@jimmidyson
Copy link
Contributor Author

/remove-label do-not-merge/hold

@k8s-ci-robot
Copy link
Contributor

@jimmidyson: The label(s) /remove-label do-not-merge/hold cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash

In response to this:

/remove-label do-not-merge/hold

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jimmidyson
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 10, 2020
@k8s-ci-robot k8s-ci-robot merged commit fd10623 into kubernetes-retired:master Aug 10, 2020
@irfanurrehman
Copy link
Contributor

@irfanurrehman The golang update includes security fixes to packages such as crypto/x509 and net/http, that while I haven't reviewed in depth whether we're affected by them, the upgrade doesn't introduce any risk so I don't see why we wouldn't do the update. The update to controller-runtime includes fixes but also metrics for webhooks (important since #1263 migrated to the controller-runtime webhook server).

IMHO a good strategy to update dependencies and/or go version is when the major releases happen, or some functionality change warrants the update.

I agree if we're doing major version upgrades of dependencies, but otherwise updating to minor or patch releases where backwards compatibility applies, I would prefer to upgrade adhoc. That way the project takes advantage of latest bug fixes, security fixes, and improvements in dependencies as we go, while also reducing the need for a bigger PR to do upgrades later (smaller updates are always preferred imo).

I did not mean to indicate that this change is not welcome, if that is what it did sound like. I wanted to point out my inclination on having a good strategy for such changes. I think we did lack this strategy earlier and right now is as good a time as any.

Additionally I think it would also be a good call to put a review strategy in place (given we still have active reviewers from multiple organisations). This is important to maintain the opinion diversity/sense of community alive in the project, lest the project becomes further opinionated. For an example we had a review strategy earlier which indicated that the same organisation contributors refrained from the final lgtm on the PR and atleast 2 reviewers needed to be happy for the PR to be merged. I am not saying that we force something like this stringently, but again IMO this is a good strategy as any, unless somebody has a better one.
@hectorj2f @jimmidyson @RainbowMango @makkes

@jimmidyson
Copy link
Contributor Author

@irfanurrehman Thank you for your comments and I really do apologise for what has happened with these last 2 PRs - we will make sure this doesn't happen again. I completely understand what you are saying and agree completely with you about the review strategy. @makkes (a colleague of mine) made the same comments to me when he saw what was happening and has volunteered to write up a review strategy for us to review :) to work better together. Again, apologies that this has happened but let's use it as a catalyst for improvement.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants