Skip to content
This repository was archived by the owner on Aug 14, 2020. It is now read-only.

lib: move internal packages to lib/internal and export ParseDockerURL and annotations#135

Merged
iaguis merged 5 commits intoappc:masterfrom
kinvolk-archives:iaguis/refactor-export
Mar 9, 2016
Merged

lib: move internal packages to lib/internal and export ParseDockerURL and annotations#135
iaguis merged 5 commits intoappc:masterfrom
kinvolk-archives:iaguis/refactor-export

Conversation

@iaguis
Copy link
Copy Markdown
Member

@iaguis iaguis commented Mar 9, 2016

Good things brought by this PR:

  • Internal functions are clearly separated from the rest
  • People can use annotations from outside docker2aci
  • We weren't exporting ParseDockerURL which is used by rkt, fix that

Fixes #133

iaguis added 4 commits March 9, 2016 12:40
To allow that, move docker_functions.go code to a "docker" package and
make the exported ParseDockerURL call the internal ParseDockerURL in the
"docker" package.
@iaguis iaguis force-pushed the iaguis/refactor-export branch from b6a183d to bce539b Compare March 9, 2016 11:41
ParsedDockerURL was part of the internal package so a user that wants to
write a function using that type can't do it because they're not able to
import the internal package.

Define the type docker.ParsedDockerURL as common.ParsedDockerURL.
Comment thread lib/common/common.go Outdated

// ParseDockerURL takes a Docker URL and returns a ParsedDockerURL with its
// index URL, image name, and tag.
func ParseDockerURL(arg string) (*types.ParsedDockerURL, error) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is troublesome - the returned result is a part of internal package, so the user of the ParseDockerURL function can use it directly, but cannot write a function that takes the *types.ParsedDockerURL, because the user will not be able to import the internal package…

I think that putting the ParsedDockerURL struct in the common package will end up in cyclic dependencies again, so consider adding type ParsedDockerURL types.ParsedDockerURL somewhere in the common package and do some casting in the ParseDockerURL function.

@krnowak
Copy link
Copy Markdown
Member

krnowak commented Mar 9, 2016

Something to consider before releasing 1.0 in a separate PR - replace the pkg/log with a Logger field in the CommonConfig struct, so users can pass their own logger. The Logger fields would be of some interface type like:

type Logger interface {
    Debug(i ...interface{})
    Info(i ...interface{})
}

so the implementer could implement the Debug function in the same way as currently pkg/log does without resorting to some global boolean variable set by the InitDebug() function. Possibly export two implementation like DefaultLogger with configurable debugging function and DevNullLogger.

@krnowak
Copy link
Copy Markdown
Member

krnowak commented Mar 9, 2016

Alright, I think I'll put my comment into a separate issue.

LFAD.

@krnowak
Copy link
Copy Markdown
Member

krnowak commented Mar 9, 2016

Filed #136.

@iaguis
Copy link
Copy Markdown
Member Author

iaguis commented Mar 9, 2016

Thanks!

iaguis added a commit that referenced this pull request Mar 9, 2016
lib: move internal packages to lib/internal and export ParseDockerURL and annotations
@iaguis iaguis merged commit 4e4aced into appc:master Mar 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export annotation keys (e.g. appcDockerEntrypoint, appcDockerCmd, etc)

2 participants