Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Extract: Detect app in cwd and subdirs. #51

Merged
merged 1 commit into from
Apr 27, 2018
Merged

Extract: Detect app in cwd and subdirs. #51

merged 1 commit into from
Apr 27, 2018

Conversation

mnottale
Copy link
Contributor

Signed-off-by: Matthieu Nottale matthieu.nottale@docker.com

}
appname = path.Join(cwd, hit)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can this be put in a fonction maybe?

for _, c := range content {
if strings.HasSuffix(c.Name(), constants.AppExtension) {
if hit != "" {
return "", nil, fmt.Errorf("multiple apps found in current directory")
Copy link
Member

Choose a reason for hiding this comment

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

In that case we should probably suggest to the user how to proceed (e.g. specify the app on the command line)?

app := ""
if len(args) > 0 {
app = args[0]
}
Copy link
Member

@mat007 mat007 Apr 27, 2018

Choose a reason for hiding this comment

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

Did that fail or is it just in case? Because I don't believe it is possible for args[0] not to contain the application name, that is if cobra args are actually os.Args of course.

Copy link
Member

Choose a reason for hiding this comment

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

Right this is actually the first arg of the command, nothing to do with the application name… Never mind…

@mnottale
Copy link
Contributor Author

@mat007 split in a subfunction and improved error message

if len(args) > 0 {
app = args[0]
}
err := packager.Unpack(app, unpackOutputDir)
Copy link
Member

Choose a reason for hiding this comment

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

nb: this could be factorized, maybe something like

err := packager.Unpack(firstArg(args), unpackOutpuDir)

and

func firstArg(args []string) string {
	if len(args) > 0 {
		return args[0]
	}
	return ""
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, but will be in a followup though as it conflicts with an other PR in my stack.

@@ -14,9 +18,45 @@ var (
noop = func() {}
)

// FindApp looks for an app in CWD or subdirs
func FindApp() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This does not actually need to be exported, does it? So findApp?

Signed-off-by: Matthieu Nottale <matthieu.nottale@docker.com>
Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

Couple nits but otherwise LGTM

func Extract(appname string) (string, func(), error) {
if appname == "" {
var err error
appname, err = findApp()
Copy link
Member

Choose a reason for hiding this comment

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

nit: think you can do:

var err error
if appname, err = findApp(); err != nil {
...

Copy link
Member

Choose a reason for hiding this comment

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

Same further down:

if err = extract(appname, tempDir); err != nil {
...

@mnottale mnottale merged commit 70cde29 into docker:master Apr 27, 2018
@mnottale mnottale deleted the detect-app-cwd branch April 27, 2018 14:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants