Update logrus to v1.0.1 (Sirupsen -> sirupsen) by dmcgowan · Pull Request #34272 · moby/moby

dmcgowan · 2017-07-26T23:21:45Z

Alternative to #33998

This handle the case sensitivity issue with logrus changing names. This is currently using manual edits on some of the changes in the vendor directory until the vendored projects merge their associated changes. This change aims to have minimal impact in terms of new vendored code. Each of the individual libraries should have changes after the updates get merged.

See https://github.com/sirupsen/logrus#logrus- for the reasoning behind this change

Related PRs

AkihiroSuda · 2017-07-27T02:33:14Z

I modified the title of this PR from Update logrus to v1.0.1 to Update logrus to v1.0.1 (Sirupsen -> sirupsen) so that 3rd parties importing client pkg can easily find this PR, but please feel free to revert 😄

dnephin

LGTM

aaronlehmann · 2017-07-27T16:17:57Z

I'm confused about how this passes CI. I'd expect the "dirty" vendor directory to fail the vendor validation check, and I don't see changes to the validation scripts to make an exception.

dnephin · 2017-07-27T16:28:03Z

hmm, good question. It looks like the vendor job didn't run on this.

dmcgowan · 2017-07-28T00:22:07Z

Updated to make the vendor done more cleanly. It is using forks where the upstreams have not been merged. The forks are based on the version which was previously there with only a commit to rename the imports. When the upstreams are merged and go to update the corresponding vendor, then updated imports should work fine. If any of these dependencies try to update before merging the logrus change upstream (they are all docker-related projects), then they will have to fix logrus themselves or merge the change (they will not have my sympathy).

dnephin · 2017-07-28T00:44:10Z

Any chance your branch can be opened as a PR against those repos ? If it doesn't merge cleanly maybe just an issue to track the change?

lowenna · 2017-08-01T01:32:56Z

Restart z again as the previous run had hit at least one failure:

18:26:25 ----------------------------------------------------------------------
18:26:25 FAIL: docker_cli_daemon_test.go:1416: DockerDaemonSuite.TestCleanupMountsAfterDaemonAndContainerKill
18:26:25 
18:26:25 [d81792b14d8fa] waiting for daemon to start
18:26:25 [d81792b14d8fa] daemon started
18:26:25 
18:26:25 docker_cli_daemon_test.go:1437:
18:26:25 [d81792b14d8fa] exiting daemon
18:26:25     // restart daemon.
18:26:25     d.Restart(c)
18:26:25 daemon/daemon.go:392:
18:26:25     t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
18:26:25 ... Error: Error while stopping the daemon d81792b14d8fa : signal: killed
18:26:25 
18:26:29 
18:26:29 ----------------------------------------------------------------------

vdemeester

@dmcgowan let's update the vendor.conf to not point on forks for opengcs, runc and containerd (with containerd/containerd#1252 merged) and use forks for libnetwork and swarmkit to deal with the chicken-and-egg problem

vdemeester · 2017-08-01T09:51:35Z

vendor.conf

 github.com/gorilla/context v1.1
 github.com/gorilla/mux v1.1
-github.com/jhowardmsft/opengcs v0.0.9
+github.com/jhowardmsft/opengcs b9d0120d36f26e981a50bf18bac1bb3f0c2b8fef https://github.com/dmcgowan/opengcs.git


@dmcgowan PR has been merge, we should update to not using the fork anymore 👼

Merged but cannot use master yet, see conversation with @jhowardmsft about a follow up to bring it to master. I am using a fork here to avoid pulling in unrelated changes of which I know little about. My expectation is those that better understand these projects will be able to get off my forks by opening a follow up updating to the most recent.

@vdemeester Agreed with @dmcgowan's comment ^^. I have exactly that follow-up PR I'm about to submit which will be backed up behind this one being merged, and move back to jhowardmsft/opengcs, but at v0.0.12, which incorporates lower-case sirupsen

ok sounds fair 👼

vdemeester · 2017-08-01T09:51:48Z

vendor.conf


 # When updating, also update RUNC_COMMIT in hack/dockerfile/binaries-commits accordingly
-github.com/opencontainers/runc 2d41c047c83e09a6d61d464906feb2a2f3c52aa4 https://github.com/docker/runc
+github.com/opencontainers/runc e9325d442f5979c4f79bfa9e09bdf7abb74ba03b https://github.com/dmcgowan/runc.git


Same here 👼

Same here about follow ups, I have already discussed with @crosbymichael about getting us back onto github.com/opencontainers/runc after this change.

vdemeester · 2017-08-01T09:52:09Z

vendor.conf


 # containerd
-github.com/containerd/containerd 3addd840653146c90a254301d6c3a663c7fd6429
+github.com/containerd/containerd fc10004571bb9b26695ccbf2dd4a83213f60b93e https://github.com/dmcgowan/containerd.git


We should wait for containerd/containerd#1252 to get merged before merging this one 👼

It's merged and can be updated here 👍

Same, this should be done in follow up so the containerd related changes can be reviewed separately. My fork only applies the logrus changes on top of the previous hash

lowenna · 2017-08-01T20:27:35Z

Restarted z CI (yet again...)

dmcgowan · 2017-08-01T20:35:03Z

@vdemeester I probably should have outlined the plan here better. The points of these forks is intended to be temporary since updating the dependencies will require removing my fork to pick up any functionality changes. I have tried to link to all the other work not as blockers to get this in, but to show the follow up work is in progress to get us back to the ideal fork-less state. Unfortunately this is a much messier change anyone would have liked and merging it will help us move past it quicker 😄

vdemeester · 2017-08-01T21:00:00Z

@dmcgowan sounds good 👍 Let's merge this 👼

johnstep

LGTM

thaJeztah · 2017-08-01T21:19:35Z

Thanks for explaining @dmcgowan - @vdemeester and I were discussing this morning, and thought it was best to first have the upstreams merged, so that we would be sure it would be accepted upstream; LGTM with that explanation 👍

lowenna · 2017-08-01T23:31:25Z

And restarted z again. Failed almost 3 hours in. @thaJeztah Who can whack the z nodes into submission? 7, for example, seems to be running under half the speed of node 1.

(Latest iteration is running on node 1 which seems to be one of the most reliable of the current set of nodes)

thaJeztah · 2017-08-02T06:52:02Z

ping @tophj-ibm ^^

tonistiigi · 2017-08-14T20:57:28Z

@dmcgowan Is there an issue tracking the docker/cli update. I don't see it referenced here.

vdemeester · 2017-08-14T21:00:12Z

@tonistiigi @simonferquel created a PR for it 👼

dmcgowan requested review from AkihiroSuda, cpuguy83, dnephin, johnstep, lowenna and tonistiigi as code owners July 26, 2017 23:21

GordonTheTurtle added the status/0-triage label Jul 26, 2017

AkihiroSuda changed the title ~~Update logrus to v1.0.1~~ Update logrus to v1.0.1 (Sirupsen -> sirupsen) Jul 27, 2017

AkihiroSuda added rebuild/z status/2-code-review and removed status/0-triage labels Jul 27, 2017

GordonTheTurtle removed the rebuild/z label Jul 27, 2017

AkihiroSuda mentioned this pull request Jul 27, 2017

update to github.com/sirupsen/logrus v1.0.0 #33998

Closed

dnephin approved these changes Jul 27, 2017

View reviewed changes

dnephin added the rebuild/vendor label Jul 27, 2017

GordonTheTurtle removed the rebuild/vendor label Jul 27, 2017

dmcgowan force-pushed the update-logrus branch from 0bf9bc2 to 2345c66 Compare July 27, 2017 20:48

dmcgowan added the rebuild/vendor label Jul 27, 2017

GordonTheTurtle removed the rebuild/vendor label Jul 27, 2017

dmcgowan force-pushed the update-logrus branch from 2345c66 to 24087a4 Compare July 27, 2017 20:54

dmcgowan added the rebuild/vendor label Jul 27, 2017

GordonTheTurtle removed the rebuild/vendor label Jul 27, 2017

dmcgowan force-pushed the update-logrus branch from 24087a4 to 132d135 Compare July 27, 2017 21:48

dmcgowan added the rebuild/vendor label Jul 27, 2017

GordonTheTurtle removed the rebuild/vendor label Jul 27, 2017

lowenna added status/4-merge and removed status/2-code-review labels Aug 1, 2017

lowenna approved these changes Aug 1, 2017

View reviewed changes

thaJeztah added the impact/changelog label Aug 1, 2017

vdemeester suggested changes Aug 1, 2017

View reviewed changes

justincormack approved these changes Aug 1, 2017

View reviewed changes

thaJeztah added status/2-code-review and removed status/4-merge labels Aug 1, 2017

vdemeester approved these changes Aug 1, 2017

View reviewed changes

vdemeester added status/4-merge and removed status/2-code-review labels Aug 1, 2017

johnstep approved these changes Aug 1, 2017

View reviewed changes

lowenna mentioned this pull request Aug 1, 2017

Windows: named pipe mounts #33852

Merged

lowenna merged commit 8af4db6 into moby:master Aug 2, 2017

Random-Liu mentioned this pull request Aug 2, 2017

Change all Sirupsen to sirupsen containerd/cri#106

Closed

3 tasks

aaronlehmann mentioned this pull request Aug 3, 2017

[WIP] update swarmkit dependecies to support plugable secrets #34331

Merged

mavenugo mentioned this pull request Aug 5, 2017

Update logrus to v1.0.1 moby/libnetwork#1856

Merged

XenoPhex mentioned this pull request Aug 23, 2017

Fix 'sirupsen/logrus' import statements #33641

Closed

8 tasks

bboreham mentioned this pull request Oct 18, 2017

[WIP] Upgrade weaveworks/common and lower Sirupsen/logrus weaveworks/scope#2881

Closed

ollanta mentioned this pull request Nov 5, 2019

Update to latest dependencies Storytel/go-docker-initiator#30

Merged

thaJeztah mentioned this pull request Apr 24, 2024

github.com/Sirupsen/logrus: github.com/Sirupsen/logrus@v1.8.1: parsing go.mod: module declares its path as: github.com/sirupsen/logrus but was required as: github.com/Sirupsen/logrus #42939

Open

Conversation

dmcgowan commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Jul 27, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Jul 27, 2017

Uh oh!

dnephin commented Jul 27, 2017

Uh oh!

dmcgowan commented Jul 28, 2017

Uh oh!

dnephin commented Jul 28, 2017

Uh oh!

lowenna commented Aug 1, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowenna commented Aug 1, 2017

Uh oh!

dmcgowan commented Aug 1, 2017

Uh oh!

vdemeester commented Aug 1, 2017

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 1, 2017

Uh oh!

lowenna commented Aug 1, 2017

Uh oh!

thaJeztah commented Aug 2, 2017

Uh oh!

tonistiigi commented Aug 14, 2017

Uh oh!

vdemeester commented Aug 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

dmcgowan commented Jul 26, 2017 •

edited

Loading