Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decoupled crane logic from crane command line code #476

Closed
wants to merge 2 commits into from
Closed

Decoupled crane logic from crane command line code #476

wants to merge 2 commits into from

Conversation

mschwrz
Copy link

@mschwrz mschwrz commented Jun 29, 2019

  • Added commands package
  • Added api package
  • Extracted logic from commands into api package

This should resolve #407

  - Added commands package
  - Added api package
  - Extracted logic from commands into api package
@mschwrz mschwrz changed the title Decoupled crane logic from crane command line code (#407) Decoupled crane logic from crane command line code Jun 29, 2019
@codecov-io
Copy link

codecov-io commented Jun 29, 2019

Codecov Report

Merging #476 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #476   +/-   ##
=======================================
  Coverage   62.84%   62.84%           
=======================================
  Files          76       76           
  Lines        3722     3722           
=======================================
  Hits         2339     2339           
  Misses       1074     1074           
  Partials      309      309

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca8b66c...14511ec. Read the comment docs.

@jonjohnsonjr
Copy link
Collaborator

Oof, I wish I had seen your comment yesterday. I had been working on this recently, and it's slightly different from what you had here. I've rebased that and pushed it here: #478

Instead of doing a direct CLI -> package translation, I tried to tease apart what crane does into a set of functions that is usable without having to spend too much time learning the abstractions in this library, e.g.:

$ crane pull == crane.Save(crane.Pull(...))

$ crane export == crane.Export(crane.Pull(...))

$ crane push == crane.Push(crane.Load(...))

because this would allow someone who wanted to export the filesystem of a tarball image to easily discover they could do so via:

crane.Export(crane.Load(...))

Apologies for not responding -- I don't mean to waste your time. I'd love your feedback on my PR if you're interested 😄

@mschwrz
Copy link
Author

mschwrz commented Jun 30, 2019

No Problem, I just could have waited for your response before starting to work on it 😄 The time isn't wasted at all, I'm currently learning go and was searching for some practical examples.

I like your approach in solving this issue. I also considered coding it like that, but discarded it.
I discarded it because I saw that ko kept its ko/cmd/ko directory very clean by only putting the main-logic into it and placed the commands themselves into the pkg/commands directory.
With this approach, ko nicely decoupled the commands from the main logic.

Maybe we can try something like this:

  • The "Business Logic" of crane goes into pkg/crane, so the user can still use crane.foo(), without having to rename pkg/crane/api to crane on importing it, as you designed it in your PR
  • The command logic goes into pkg/crane/commands, to keep the cmd/crane directory clean

@jonjohnsonjr
Copy link
Collaborator

I think what we have is pretty close to this now, going to close this PR but feel free to reopen if I missed something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple pkg/crane from command line flags
3 participants