Bugfix/2935 lakectl bug on not found html #2966

idanovo · 2022-02-24T17:07:28Z

No description provided.

BinyaminSchein · 2022-02-24T19:29:06Z

Loving thisssss

…esponse

arielshaqed

Thanks!

1 res/resp mixup that has to be fixed, and some things don't belong in this PR I think.

arielshaqed · 2022-02-27T11:58:18Z

cmd/lakectl/cmd/annotate.go

 			for _, obj := range resp.JSON200.Results {
 				logCommitsParams := &api.LogCommitsParams{
 					Amount:  api.PaginationAmountPtr(1),
 					Objects: &[]string{obj.Path},
 				}
 				res, err := client.LogCommitsWithResponse(context, pathURI.Repository, pathURI.Ref, logCommitsParams)
-				DieOnResponseError(res, err)
+				DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusOK)


This is actually incorrect, because l.51 assigned to res. Response variables are clearly a mess in here. Right now you're the victim, and the only way out might be to fix them entirely.

If you can, I would feel safely if you could: pick one spelling, and search-and-destroy the entire cmd directory for the other one (git grep -w res cmd/ is your friend, and should return no results once we're done.)

tag.go still contains a variable res:

[ariels@fedora lakeFS]$ git grep -w res cmd/ cmd/lakectl/cmd/tag.go: res, err := client.GetCommitWithResponse(ctx, tagURI.Repository, commitRef) cmd/lakectl/cmd/tag.go: DieOnErrorOrUnexpectedStatusCode(res, err, http.StatusOK)

This inconsistency is precisely what got us into trouble on this line. So could you fix that one too, please, even though it is not strictly related to this PR?

My 2 cents - use different names so we will not verify or check the wrong response by mistake

arielshaqed · 2022-02-27T12:00:39Z

cmd/lakectl/cmd/annotate.go

 				data := objectCommitData{
 					Object: obj.Path,
 				}
 				if len(res.JSON200.Results) > 0 {
 					data.Commit = res.JSON200.Results[0]
-					data.CommitMessage = splitOnNewLine(stringTrimLen((res.JSON200.Results[0].Message), annotateMessageSize))
+					data.CommitMessage = splitOnNewLine(stringTrimLen(res.JSON200.Results[0].Message, annotateMessageSize))


This seems unrelated. Also I am not sure it does what is intended here (I am not sure what is intended here). Maybe leave it to a separate PR?

arielshaqed · 2022-02-27T12:02:06Z

cmd/lakectl/cmd/common_helpers.go

@@ -206,6 +210,31 @@ func DieOnResponseError(response interface{}, err error) {
 		DieErr(retrievedErr)
 	}
 }
+func DieOnErrorOrUnexpectedStatusCode(response interface{}, err error, expectedStatusCode int) {


Could we refactor parts of HTTPResponseAsError with this? I think it copies some parts.

I've tried to, but it seems to be kind of difficult because of the structure of ResponseAsError func. All we want in our case is to get the status code from the response when ResponseAsError checks and returns an error on its way to get it. WDYT?

arielshaqed · 2022-02-27T12:02:32Z

cmd/lakectl/cmd/doctor.go

@@ -127,7 +128,7 @@ func ListRepositoriesAndAnalyze(ctx context.Context) error {
 	case resp.JSON401 != nil:
 		return &CredentialsError{msgOnErrCredential, resp.JSON401.Message}
 	// In case we get the "not found" HTML page (the status is 200 and not 404 in this case).
-	case resp.HTTPResponse != nil && resp.HTTPResponse.StatusCode == 200:
+	case resp.HTTPResponse != nil && resp.HTTPResponse.StatusCode == 302:


The comment doesn't change?

Which comment? The output comment of the doctor?

arielshaqed · 2022-02-27T12:03:40Z

docs/reference/commands.md

@@ -1339,7 +1339,7 @@ lakectl branch-protect add <repo uri> <pattern> [flags]
 {:.no_toc}

 ```
-lakectl add lakefs://<repository> 'stable_*'
+lakectl branch-protect add lakefs://<repository> 'stable_*'


These seem unrelated, and I'd rather the author reviewed this.

Opened #2976

arielshaqed

Thanks!

arielshaqed · 2022-02-27T13:12:07Z

cmd/lakectl/cmd/annotate.go

 			for _, obj := range resp.JSON200.Results {
 				logCommitsParams := &api.LogCommitsParams{
 					Amount:  api.PaginationAmountPtr(1),
 					Objects: &[]string{obj.Path},
 				}
 				res, err := client.LogCommitsWithResponse(context, pathURI.Repository, pathURI.Ref, logCommitsParams)
-				DieOnResponseError(res, err)
+				DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusOK)


tag.go still contains a variable res:

[ariels@fedora lakeFS]$ git grep -w res cmd/ cmd/lakectl/cmd/tag.go: res, err := client.GetCommitWithResponse(ctx, tagURI.Repository, commitRef) cmd/lakectl/cmd/tag.go: DieOnErrorOrUnexpectedStatusCode(res, err, http.StatusOK)

This inconsistency is precisely what got us into trouble on this line. So could you fix that one too, please, even though it is not strictly related to this PR?

arielshaqed · 2022-02-27T13:14:23Z

cmd/lakectl/cmd/tag.go

@@ -78,8 +78,8 @@ var tagCreateCmd = &cobra.Command{
 		force, _ := cmd.Flags().GetBool("force")
 		if force {
 			// checking validity of the commitRef before deleting the old one
-			resp, err := client.GetCommitWithResponse(ctx, tagURI.Repository, commitRef)
-			DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusOK)
+			res, err := client.GetCommitWithResponse(ctx, tagURI.Repository, commitRef)


Sorry for being unclear: I really liked this change, it's part of the res -> resp fix. It caused an issue with this PR, so I think we have no choice but to make this unrelated change here. (Technically we could have a preview PR that just fixed this, and then your PR. Too much effort for too little gain probably.)

not sure how we can fix this in this case. We are trying to assign resp in l.84 with a different type. Maybe change res here to be _resp?

nopcoder · 2022-02-27T10:47:02Z

cmd/lakectl/cmd/branch_protect.go

@@ -18,13 +21,13 @@ var branchProtectCmd = &cobra.Command{
 var branchProtectListCmd = &cobra.Command{
 	Use:     "list <repo uri>",
 	Short:   "List all branch protection rules",
-	Example: "lakectl list lakefs://<repository>",
+	Example: "lakectl branch-protect list lakefs://<repository>",


nopcoder · 2022-02-27T14:25:03Z

cmd/lakectl/cmd/abuse.go

@@ -119,7 +120,7 @@ var abuseRandomWritesCmd = &cobra.Command{

 		client := getClient()
 		resp, err := client.GetRepositoryWithResponse(cmd.Context(), u.Repository)
-		DieOnResponseError(resp, err)
+		DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusOK)


suggestion - pick a shorter name

DieOnUnexpectedResponse?

Sorry, no good suggestion

nopcoder · 2022-02-27T14:29:13Z

cmd/lakectl/cmd/annotate.go

 			for _, obj := range resp.JSON200.Results {
 				logCommitsParams := &api.LogCommitsParams{
 					Amount:  api.PaginationAmountPtr(1),
 					Objects: &[]string{obj.Path},
 				}
 				res, err := client.LogCommitsWithResponse(context, pathURI.Repository, pathURI.Ref, logCommitsParams)
-				DieOnResponseError(res, err)
+				DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusOK)


My 2 cents - use different names so we will not verify or check the wrong response by mistake

nopcoder · 2022-02-27T14:29:49Z

cmd/lakectl/cmd/branch_protect.go

 	"github.com/spf13/cobra"
+


remove whitespace

nopcoder · 2022-02-27T14:31:28Z

cmd/lakectl/cmd/common_helpers.go

@@ -41,6 +43,8 @@ const (
 const internalPageSize = 1000          // when retreiving all records, use this page size under the hood
 const defaultAmountArgumentValue = 100 // when no amount is specified, use this value for the argument

+const defaultResponseOnSwaggerClient = http.StatusNoContent


Prefer to use http.StatusNoContent when needed - the code will be readable without this indirection

nopcoder · 2022-02-27T14:33:58Z

cmd/lakectl/cmd/root.go

@@ -108,9 +109,18 @@ func getClient() *api.ClientWithResponses {
 	if u.Path == "" || u.Path == "/" {
 		serverEndpoint = strings.TrimRight(serverEndpoint, "/") + api.BaseURL
 	}
+	httpClient := &http.Client{
+


remove whitespace

nopcoder · 2022-02-27T14:35:19Z

pkg/api/ui_handler.go

@@ -80,6 +80,8 @@ func NewHandlerWithDefault(root http.FileSystem, handler http.Handler, defaultPa
 		}
 		_, err := root.Open(path.Clean(urlPath))
 		if err != nil && os.IsNotExist(err) {
+			fmt.Println("redirecting...")


remove println

nopcoder · 2022-02-27T14:36:07Z

pkg/api/ui_handler.go

@@ -80,6 +80,8 @@ func NewHandlerWithDefault(root http.FileSystem, handler http.Handler, defaultPa
 		}
 		_, err := root.Open(path.Clean(urlPath))
 		if err != nil && os.IsNotExist(err) {
+			fmt.Println("redirecting...")
+			http.Redirect(w, r, defaultPath, http.StatusFound)


As far as I remember, you need to return after redirect and not continue the code flow

nopcoder

Just address other comments

idanovo added 2 commits February 24, 2022 14:29

avoiding redirect automatically

855e9ce

Changed all CLI commands to fail on unexpected status code returned

704cacd

idanovo added bug Something isn't working good first issue Good for newcomers area/lakectl Issues related to lakeFS' command line interface (lakectl) include-changelog PR description should be included in next release changelog labels Feb 24, 2022

idanovo self-assigned this Feb 24, 2022

idanovo added 2 commits February 24, 2022 19:32

Format fixes

3066a9c

Fixed expected status for git repository create with bare flag

574c765

idanovo added 3 commits February 25, 2022 12:02

Fixed Lakectl doctor commmand due to changes in changes on redirect r…

f7bb6ea

…esponse

Reformated import order

3160b53

Fixed expected status for branch protected add example text

08582c2

idanovo requested review from nopcoder, arielshaqed, Jonathan-Rosenberg, talSofer and itaidavid February 27, 2022 10:36

idanovo marked this pull request as ready for review February 27, 2022 11:12

arielshaqed requested changes Feb 27, 2022

View reviewed changes

Reverted unrelated changes to current PR

360d462

idanovo requested a review from arielshaqed February 27, 2022 13:02

Reverted changes

3513f29

arielshaqed approved these changes Feb 27, 2022

View reviewed changes

arielshaqed force-pushed the bugfix/2935-lakectl-bug-on-not-found-html branch from 284669d to 3513f29 Compare February 27, 2022 13:48

idanovo linked an issue Feb 27, 2022 that may be closed by this pull request

BUG: lakectl get panic when the response is the not found html page #2935

Closed

nopcoder suggested changes Feb 27, 2022

View reviewed changes

Fixed imports order and responses naming

688de92

idanovo requested a review from nopcoder February 28, 2022 09:32

nopcoder approved these changes Feb 28, 2022

View reviewed changes

idanovo merged commit a8340bf into master Feb 28, 2022

arielshaqed deleted the bugfix/2935-lakectl-bug-on-not-found-html branch May 8, 2022 07:49

arielshaqed restored the bugfix/2935-lakectl-bug-on-not-found-html branch May 8, 2022 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/2935 lakectl bug on not found html #2966

Bugfix/2935 lakectl bug on not found html #2966

idanovo commented Feb 24, 2022

BinyaminSchein commented Feb 24, 2022

arielshaqed left a comment

arielshaqed Feb 27, 2022

arielshaqed Feb 27, 2022

nopcoder Feb 27, 2022

arielshaqed Feb 27, 2022

arielshaqed Feb 27, 2022

idanovo Feb 27, 2022

arielshaqed Feb 27, 2022

idanovo Feb 27, 2022

arielshaqed Feb 27, 2022

idanovo Feb 27, 2022

arielshaqed left a comment

arielshaqed Feb 27, 2022

arielshaqed Feb 27, 2022

idanovo Feb 27, 2022

nopcoder Feb 27, 2022

nopcoder Feb 27, 2022

idanovo Feb 28, 2022

nopcoder Feb 28, 2022

nopcoder Feb 27, 2022

nopcoder Feb 27, 2022

nopcoder Feb 27, 2022

nopcoder Feb 27, 2022

nopcoder Feb 27, 2022

nopcoder Feb 27, 2022

nopcoder left a comment

Bugfix/2935 lakectl bug on not found html #2966

Bugfix/2935 lakectl bug on not found html #2966

Conversation

idanovo commented Feb 24, 2022

BinyaminSchein commented Feb 24, 2022

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment