Skip to content

Cleanup: server image#7427

Closed
tiborvass wants to merge 10 commits intomoby:masterfrom
tiborvass:cleanup-server-image
Closed

Cleanup: server image#7427
tiborvass wants to merge 10 commits intomoby:masterfrom
tiborvass:cleanup-server-image

Conversation

@tiborvass
Copy link
Copy Markdown
Contributor

@tiborvass tiborvass commented Aug 5, 2014

This PR removes all the image jobs from server, into graph/. There is one exception: image_delete which still has a dependency on the daemon (to be fixed); that has been moved to daemon/.

All this work was done by @shykes. I simply resolved rebase conflicts and migrated tests.

Ping @crosbymichael @LK4D4 @unclejack @erikh

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

Thanks Tibor!

@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Aug 6, 2014

I don't like separate file for every action. Also, go to definition doesn't work with jobs, so I need to inspect all graph dir content to know where I can find my precious piece of code. Now, I remembered that for thing I need just press Ctrl-P and find graphservice. I think that it will be hard for me to remember 7 different names, but maybe this is just my problem.

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

@LK4D4 separate file for every action is so much nicer than one file with 2k lines

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

@tiborvass @shykes

These PRs are hard to review so can we trust you that this did not change functionally in an external way and that it is just refactoring and moving code around?

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

What makes them hard to review specifically? Anything I can do to fix that?

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

@shykes no, it's just the big blobs of green in the github UI. Nothing you guys did wrong. ;)

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

@LK4D4 it doesn't have to stay 1-file-per-action forever, but for this particular operation (transcribing large amounts of code across directories) it definitely helps.

@tiborvass
Copy link
Copy Markdown
Contributor Author

tiborvass commented Aug 6, 2014

@crosbymichael @shykes I could rearrange this adding one commit before all those, which would be without moving the blocks of codes, so that you can see a clean diff in the same file (but it would not compile), and then have each of those "Move" commits be a trivial cut/paste.

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

@tiborvass no it's ok.

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

@crosbymichael the methodology was to change as little as possible in the actual content. In most cases that was:

  • move the method to a new file in graph
  • sometimes rename the method for consistency (eg CmdXXX)
  • remove references to Server in the method
  • Move job registration from server/init.go to graph/service.go
  • When relevant, move existing code from graph/ to the new file (for example there are 2 competing tag implementations, they are now in the same file so we can look at them and decide what to do)

I will try to point out exceptions to this in line comments.

@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Aug 6, 2014

Just worked with splitted to 40 files daemon. This was awful. Such code arranging will slow my development I think. I agreed that 2k lines is too much, but 40 files with 20-40 lines of code is other extreme which is not better. I think 600-1000 lines is still okay and there are many such files in go standard library for example.
I'll try to get used with this, but I was needed to talk to someone about it :D

crosbymichael
crosbymichael reviewed Aug 6, 2014
View reviewed changes
graph/service.go Outdated
Copy link
Copy Markdown
Contributor

@crosbymichael crosbymichael Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we are not checking errors returned from register we do not know if this refactoring is overwriting any jobs already registered with the same name throughout the code base.

Do you think we can check the errors from register?

Copy link
Copy Markdown
Contributor Author

@tiborvass tiborvass Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that.

Copy link
Copy Markdown
Contributor

@shykes shykes Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I recommend going through a loop a-la daemon.Daemon.Install

Copy link
Copy Markdown
Contributor Author

@tiborvass tiborvass Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

@LK4D4 really I'm not saying we should switch everything to lots of little files. It just seemed right to better manage a big copy-paste operation with potential merge conflicts etc.

@tiborvass
Copy link
Copy Markdown
Contributor Author

tiborvass commented Aug 6, 2014

@crosbymichael @shykes updated.

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

Travis is failing now

Solomon Hykes and others added 10 commits August 6, 2014 17:18
@tiborvass
Move "image_export" to graph/export.go
6e28d11 
Signed-off-by: Solomon Hykes <solomon@docker.com>
@tiborvass
Move "image_history" to graph/history.go
55235e1 
Signed-off-by: Solomon Hykes <solomon@docker.com>
@tiborvass
Move "images" to graph/list.go
51dd68d 
Signed-off-by: Solomon Hykes <solomon@docker.com>
@tiborvass
Move "viz" to graph/viz.go
7778144 
Signed-off-by: Solomon Hykes <solomon@docker.com>
@tiborvass
Move "load" to graph/load.go
f2029f7 
Signed-off-by: Solomon Hykes <solomon@docker.com>
@tiborvass
Move "import" to graph/import.go
fa27580 
Signed-off-by: Solomon Hykes <solomon@docker.com>
@tiborvass
Move "image_delete" to daemon/image_delete.go
7a5e3df 
Note: this cannot yet be moved to graph/ because of a lingering
dependency on daemon. This has been noted in a FIXME.

Signed-off-by: Solomon Hykes <solomon@docker.com>
@tiborvass
Move "image_tag" and "tag" to graph/tag.go
d787937 
Note: these 2 jobs should be merged into one. This was noted in a FIXME.

Signed-off-by: Solomon Hykes <solomon@docker.com>
@tiborvass
Move some integration tests to integration-cli
69f9d48 
Signed-off-by: Tibor Vass <teabee89@gmail.com>
@tiborvass
Check error returned by eng.Register() in TagStore.Install()
af07819 
Signed-off-by: Tibor Vass <teabee89@gmail.com>
@tiborvass
Copy link
Copy Markdown
Contributor Author

tiborvass commented Aug 6, 2014

@crosbymichael sorry about that, it should be good now!

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

LGTM

@tiborvass
Copy link
Copy Markdown
Contributor Author

tiborvass commented Aug 6, 2014

Ping @unclejack

@shykes shykes mentioned this pull request Aug 6, 2014
Closed
@shykes shykes self-assigned this Aug 6, 2014
@shykes shykes removed their assignment Aug 6, 2014
@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

Sorry wrong pr, ignore last message.

@shykes shykes mentioned this pull request Aug 6, 2014
Merged
@tiborvass
Copy link
Copy Markdown
Contributor Author

tiborvass commented Aug 6, 2014

Closing in favor of #7452

@tiborvass tiborvass closed this Aug 6, 2014
@tiborvass tiborvass deleted the cleanup-server-image branch July 17, 2019 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tiborvass @shykes @LK4D4 @crosbymichael