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

fixes #1564 usr home #1582

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

sumantro93
Copy link

Add fixes for 1564

@sumantro93 sumantro93 changed the title fixes #1564 usr /home/sumantro fixes #1564 usr home Oct 31, 2024
Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, @sumantro93 !

The code is failing to compile with:

# github.com/containers/toolbox/cmd
cmd/utils.go:30:2: user redeclared in this block
cmd/utils.go:28:2: other declaration of user
cmd/utils.go:30:2: "os/user" imported and not used
cmd/create.go:308:2: declared and not used: homeDir
cmd/create.go:309:2: declared and not used: resolvedPath
cmd/create.go:309:45: undefined: HomeDir
cmd/create.go:476:15: undefined: homeDirMountArg
cmd/initContainer.go:270:21: undefined: user
cmd/initContainer.go:580:15: undefined: user
cmd/run.go:445:17: assignment mismatch: 1 variable but user.Current returns 2 values

Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

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

Here are some things that I noticed that could help with getting the code to compile:

src/cmd/utils.go Outdated
@@ -25,6 +25,7 @@ import (
"fmt"
"io"
"os"
"os/user"
Copy link
Member

Choose a reason for hiding this comment

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

I bet this line is causing:

cmd/utils.go:30:2: user redeclared in this block
cmd/utils.go:28:2: other declaration of user

The code was already importing os/user a few lines below, so no need to import it again. :)

Copy link
Member

Choose a reason for hiding this comment

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

Now you removed both imports, and it's causing:

cmd/utils.go:333:38: undefined: user

@@ -21,7 +21,6 @@ import (
"fmt"
"io/ioutil"
"os"
"os/user"
Copy link
Member

Choose a reason for hiding this comment

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

I bet this line is causing:

cmd/initContainer.go:270:21: undefined: user
cmd/initContainer.go:580:15: undefined: user

Why did you remove the import of os/user?

@@ -20,6 +20,7 @@ import (
"errors"
"fmt"
"os"

Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: stray newline.

Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

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

Finally, a few things about the Git metadata.

Could you please try to squash the commits into logical units that successfully build and pass the tests, instead of a series of fix-up commits? See the other commits for examples. It's possible that for this pull request there will be only one commit.

Could you please add a link to the GitHub issue at the bottom of the commit message like in the other commits?

Could you please use your full name for the commit? One way to do this is to set the GIT_AUTHOR_NAME and GIT_COMMITTER_NAME environment variables.

Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

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

It's still failing to build:

# github.com/containers/toolbox/cmd
cmd/utils.go:333:38: undefined: user
cmd/create.go:308:2: declared and not used: homeDir
cmd/create.go:309:2: declared and not used: resolvedPath
cmd/create.go:309:45: undefined: HomeDir
cmd/create.go:476:15: undefined: homeDirMountArg
cmd/run.go:445:17: assignment mismatch: 1 variable but user.Current returns 2 values

logrus.Errorf("Failed to get HOME directory: %v", err)
}
homeDir := user.HomeDir
resolvedPath, err := filepath.EvalSymlinks(HomeDir)
Copy link
Member

Choose a reason for hiding this comment

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

It should be homeDir, not HomeDir. This is causing:

cmd/create.go:308:2: declared and not used: homeDir
cmd/create.go:309:45: undefined: HomeDir

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