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

Added goopack functions into goolib #91

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nanditajaiswal
Copy link

added files

  • goolib/goopack.go
  • goolib/goopack_test.go

(the functions from goopack/goopack.go and test functions from goopack/goopack_test.go will be removed in the next PR)

Copy link

@groob groob left a comment

Choose a reason for hiding this comment

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

There are a number of changed I'd like to suggest, but they're more suitable for follow-up pull requests. I will file them as issues instead, so they can be discussed.

For example, CreatePackage requires an input and output path. It would be preferable to have these arguments be an io.Reader and io.Writer instead. The current implementation of CreatePackage (and all the helpers) assume a local filesystem, which is not always the case.

Perhaps a more appropriate course of action instead of refactoring all the existing code, is to introduce "CreateRemotePackage", which implements a OS agnostic archive. For the use case I need, local filesystem doesn't work, as well as shelling out to different tools.

As far as this change, it looks good to me.

firstGlob int
}

//CreatePackage accepts a goospec file and generates the packaged goo file
Copy link

Choose a reason for hiding this comment

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

space after // and "." at the end.

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.

2 participants