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.

Sign up for GitHub

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

Jump to bottom

refactor: helper function for getToken for cache reset + error handling #133

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

refactor: helper function for getToken for cache reset + error handling #133

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c84cb23
Update reset.go
srikarmanikonda Jun 3, 2023
12698de
Merge branch 'main' into patch-1
jacobwgillespie Jun 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions pkg/cmd/cache/reset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ import (
"github.com/spf13/cobra"
)

func getToken() (string, error) {
token := os.Getenv("DEPOT_TOKEN")
if token == "" {
token = config.GetApiToken()
}
if token == "" {
return "", fmt.Errorf("missing API token, please run `depot login`")
}
return token, nil
}

func NewCmdResetCache() *cobra.Command {
var projectID string
var token string
Expand All @@ -24,25 +35,25 @@ func NewCmdResetCache() *cobra.Command {
Use: "reset",
Short: "Reset the cache for a project",
Args: cli.RequiresMaxArgs(1),

RunE: func(cmd *cobra.Command, args []string) error {
var cwd string
if len(args) > 0 {
cwd, _ = filepath.Abs(args[0])
cwd, err := filepath.Abs(args[0])
if err != nil {
return fmt.Errorf("unable to determine absolute path: %w", err)
Comment on lines -28 to +42
Copy link
Member

Choose a reason for hiding this comment

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

The logic from before isn't really easy to follow, sorry about that. What's happening is that cache reset can take either 0 or 1 arguments. If no arguments are specified, then the cwd variable will contain an empty string "".

Then if an empty string passed to helpers.ResolveProjectID(projectID, cwd) as the cwd, that function will actually resolve the current working directory.

So we do only want to try to resolve args[0] if len(args) > 0.

Maybe to make it clearer, we could rename the cwd variable here cwdOverride or projectDir or something that would make it more clear?

We could also move the resolution of cwd, _ := filepath.Abs(...) into the helper function itself, I imagine every command has that part duplicated currently.

}

projectID := helpers.ResolveProjectID(projectID, cwd)
if projectID == "" {
return errors.Errorf("unknown project ID (run `depot init` or use --project or $DEPOT_PROJECT_ID)")
}
if projectID == "" {
return errors.Errorf("unknown project ID (run `depot init` or use --project or $DEPOT_PROJECT_ID)")
}
Comment on lines +49 to +51
Copy link
Member

Choose a reason for hiding this comment

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

I think this line is duplicated:

Suggested change
if projectID == "" {
return errors.Errorf("unknown project ID (run `depot init` or use --project or $DEPOT_PROJECT_ID)")
}


// TODO: make this a helper
if token == "" {
token = os.Getenv("DEPOT_TOKEN")
}
if token == "" {
token = config.GetApiToken()
}
if token == "" {
return fmt.Errorf("missing API token, please run `depot login`")
token, err := getToken()
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the build and bake commands are already using a helper function, you might be able to use the same one here as well:

token = helpers.ResolveToken(context.Background(), token)

if err != nil {
return err
}

client := api.NewProjectsClient()
Expand Down