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

Fix issue when using relative paths for KITOPS_HOME #382

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

amisevsk
Copy link
Contributor

Description

Since kit sometimes changes directory while packing/unpacking, using a relative path for KITOPS_HOME (or the --config parameter) would lead to the CLI using an unexpected location -- for example, running

  KITOPS_HOME=./my-kitops-storage
  kit pack -t my-model ./my-model-path

would lead to kit storing the model in ./my-model-path/my-kitops-storage instead of the expected ./my-kitops-storage.

Instead, we take the absolute path for the parameter at the beginning and use that.

Linked issues

Closes #381

Since kit sometimes changes directory while packing/unpacking, using a
relative path for KITOPS_HOME (or the --config parameter) would lead to
the CLI using an unexpected location -- for example, running

  KITOPS_HOME=./my-kitops-storage
  kit pack -t my-model ./my-model-path

would lead to kit storing the model in ./my-model-path/my-kitops-storage
instead of the expected ./my-kitops-storage.

Instead, we take the absolute path for the parameter at the
beginning and use that.
@@ -117,13 +119,21 @@ func Execute() {
func getConfigHome(opts *rootOptions) (string, error) {
if opts.configHome != "" {
output.Debugf("Using config directory from flag: %s", opts.configHome)
return opts.configHome, nil
absHome, err := filepath.Abs(opts.configHome)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is repeated. Should it be a nested helper function something like.

getAbsPath := func(path, description string) (string, error) {
		absPath, err := filepath.Abs(path)
		if err != nil {
			return "", fmt.Errorf("failed to get absolute path for %s: %w", description, err)
		}
		return absPath, nil
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like this level of repetition is just annoyingly inherent to golang (and trying to reduce it would just complicate the code).

The only part we're really repeating is handling the error, and I think I prefer to more clearly distinguish the message between "we couldn't figure out the path you passed via --context" vs the more subtle "KITOPS_HOME is set somewhere and that's messing things up."

e.g.

getAbsPath := func(...)

absHome, err := getAbsPath(opts.configHome, "--config parameter")
if err != nil {
    return err
}

vs.

absHome, err := filepath.Abs(path)
if err != nil {
    return fmt.Errorf("<message>")
}

@amisevsk amisevsk merged commit 2301093 into jozu-ai:main Jun 26, 2024
3 checks passed
@amisevsk amisevsk deleted the kitops-home-fixup branch June 26, 2024 18:39
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.

Using a relative path for KITOPS_HOME interacts strangely with context path in pack
2 participants