From glog to klog #25
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
cc @dims |
k8s-ci-robot
added
the
size/XS
|
We just ran into this with kubernetes-csi/external-provisioner (kubernetes-csi/external-provisioner#219). Please merge and prepare a new release with that change. |
| @@ -692,6 +692,7 @@ func (ctrl *ProvisionController) Run(_ <-chan struct{}) { | |||
| ctrl.leaderElectionNamespace, | |||
| strings.Replace(ctrl.provisionerName, "/", "-", -1), | |||
| ctrl.client.CoreV1(), | |||
| nil, | |||
There was a problem hiding this comment.
ctrl.client.CoordinationV1()
| @@ -692,6 +692,7 @@ func (ctrl *ProvisionController) Run(_ <-chan struct{}) { | |||
| ctrl.leaderElectionNamespace, | |||
| strings.Replace(ctrl.provisionerName, "/", "-", -1), | |||
| ctrl.client.CoreV1(), | |||
| nil, | |||
There was a problem hiding this comment.
It's not obvious why switching to glog depends on this change. It's probably related to some dependency change, but does that have to be in this PR?
There was a problem hiding this comment.
yes, master branch of this repo is following master branch k8s.io/*. so when k8s.io breaks something (i made this breaking change actually) we may need to update random things for builds to be green
There was a problem hiding this comment.
req'd for compilation on my end
There was a problem hiding this comment.
That would be an argument in favor of having Gopk.toml + Gopkg.lock file in the repo: with those present, someone who works on the source can focus on one aspect (like adding k8s.io/klog) without having to deal with unrelated changes.
There was a problem hiding this comment.
yes. I want to avoid doing that because in my experience imposing explicit version requirements for the library via a checked-in Gopkg.toml+lock can cause headaches for consumers. e.g. the Gopkg.toml will say this library needs k8s.io/* master branch but really it will work also with release-1.14...if another dependency needs release-1.14, can dep figure out to use release-1.14 or will it complain that there the needs are unsatisfiable? (bit of a contrived example)
Anyway, if master branch is supposed to track k8s.io/* master, ideally I should be periodically syncing it up, I'll look into adding at travis cron job let me know when something needs updating to reduce headaches for contributors
There was a problem hiding this comment.
You can have an empty Gopk.toml (or, better, a minimal one which just records the real constraints) and just record what's known to work in Gopkg.lock. Then people working with the source and Travis CI can use Gopkg.lock to fetch exactly the same source as the last person working on a dependency update (via dep ensure without -update). Downstream users will just ignore the Gopkg.lock.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jayunit100, wongma7 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 |
k8s-ci-robot
added
the
approved
|
Are you going to tag a release or should external-provisioner update to the master branch as part of the klog migration? |
|
I’ll tag tonight/tomorrow
…On Tue, Feb 5, 2019 at 4:56 PM Patrick Ohly ***@***.***> wrote:
Are you going to tag a release or should external-provisioner update to
the master branch as part of the klog migration?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#25 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMgP-OoLf3Ad4bWB69ySrXqi1WKHDz9Mks5vKf3dgaJpZM4ajRI7>
.
|
|
thanks guys, TBH this was part of a hackathon :) but im happy to help w/ the continued refactoring moving forward. ill be in sig-storage if anyone wants to discuss further. |
Cherry-pick #25 on release-1.13
|
Matthew Wong <[email protected]> writes:
thanks. had no idea they cannot coexist.
They can, but only if applications explicitly support that
(https://github.com/kubernetes/klog#coexisting-with-glog). For the CSI
sidecars we want to get rid of glog entirely and use `klog.InitFlags` -
that part conflicts with glog's own init.
|
Update kind to v0.5.0
Other repos depending on this w/ lots of klog deps seem to collide, this should fix that ... (specifically... this was burning my in external-storage w/ runtime errors showing up due to flag overrides...)