WKS-1193 - Use dynamic actions #10

ehl-jf · 2024-11-26T15:55:19Z

The pull request is targeting the main branch.
The code has been validated to compile successfully by running go vet ./....
The code has been formatted properly using go fmt ./....
All static analysis checks passed.
All tests have passed. If this feature is not already covered by the tests, new tests have been added.
All integration tests have passed locally as they cannot be automated yet.
All changes are detailed at the description. if not already covered at JFrog Documentation, new documentation have been added.

cyrilc-pro · 2024-11-26T16:03:34Z

.github/workflows/analysis.yml

@@ -36,6 +36,9 @@ jobs:
          go-version-file: go.mod

      - name: Run Gosec Security Scanner
-        uses: securego/[email protected]
+        uses: securego/[email protected]


Isn't gosec part of golangci-lint?
See https://golangci-lint.run/usage/linters/#gosec

Nice, we copied it as is from other plugins. We can definitely improve.

cyrilc-pro · 2024-11-26T16:13:17Z

commands/common/actions_metadata.go

+	return names
+}
+
+func (c ActionsMetadata) FindAction(actionName string, service ...string) (*model.ActionMetadata, error) {


Use an "option" pattern instead of service ...string to create optional parameters.

Example:

am.FindAction("my-action", WithApplication("my-app"))

I think is over engineering for just one option. We wont have others.

cyrilc-pro · 2024-11-27T08:50:22Z

commands/common/cmd_api.go

+		return apiError(http.StatusInternalServerError, "%+v", err)
+	}
+
+	apiEndpoint := fmt.Sprintf("%sworker/api/v%d/%s", utils.AddTrailingSlashIfNeeded(params.ServerUrl), params.ApiVersion+1, strings.Join(params.Path, "/"))


params.ApiVersion+1 seems rather awkward.
I suggest to use ApiVersion == 0 to mean default (1 now, but may change in the future) and to use any other value "as is".

This is an package internal type declared using the go iota thing.
So ApiVersion == 0 is already the default.
But we can set it to 1 explicitely so that we dont have to use +1

If it is internal, no worries. It is just weird...

cyrilc-pro · 2024-11-27T08:53:09Z

commands/common/cmd_api.go

+		return apiError(http.StatusInternalServerError, "unexpected error: %+v", err)
+	}
+
+	if slices.Index(params.OkStatuses, res.StatusCode) == -1 {


Should we provide meaningful default value for params.OkStatuses?

cyrilc-pro · 2024-11-27T08:59:10Z

commands/common/cmd_api.go

+
+	defer CloseQuietly(res.Body)
+
+	if res.ContentLength > 0 || res.ContentLength == -1 {


Suggestion: invert the condition

Suggested change

if res.ContentLength > 0 || res.ContentLength == -1 {

if res.ContentLength == 0 {

// discard

} else {

//consume

}

cyrilc-pro · 2024-11-27T09:30:45Z

commands/common/manifest.go

+)
+
+// ReadManifest reads a manifest from the working directory or from the directory provided as argument.
+func ReadManifest(dir ...string) (*model.Manifest, error) {


Avoid using a variadic function to implement optional arg

Not a big deal, used only for tests.

cyrilc-pro · 2024-11-27T09:36:53Z

commands/common/typescript.go

+	tsFieldAccessPattern   = regexp.MustCompile(`\W([A-Z][a-zA-Z0-9]+)\.[a-zA-Z0-9]+`)
+	tsTypeInTypeParameters = regexp.MustCompile(`<([A-Z][a-zA-Z0-9]+)>`)
+	tsExcludeTypes         = []string{"PlatformContext"}
+	tsUnexportedType       = regexp.MustCompile(`^(class|type|interface|enum|const)\s+[A-Za-z_$][0-9A-Za-z_$]*`)


Each line is actually trimmed before matching.

cyrilc-pro · 2024-11-27T09:40:42Z

commands/list_event_cmd.go

 			}

-			return callWorkerApiWithOutput(c, server.GetUrl(), server.GetAccessToken(), http.MethodGet, nil, http.StatusOK, queryParams, "actions")
+			return common.Print("%s", strings.Join(actions, ", "))


Suggested change

return common.Print("%s", strings.Join(actions, ", "))

return common.Print(strings.Join(actions, ", "))

Actually it causes a govet lint error, that the reason is like this.

cyrilc-pro · 2024-11-27T09:42:40Z

model/actions.go

+type ActionMetadata struct {
+	Action               Action           `json:"action"`
+	Description          string           `json:"description"`
+	SamplePayload        string           `json:"samplePayload"`


Should it be snake_case instead of camelCase?

The backend uses this case.

cyrilc-pro · 2024-11-27T09:43:44Z

model/flags.go

+func GetApplicationFlag() components.StringFlag {
+	return components.NewStringFlag(
+		FlagApplication,
+		"The application that provides the event. If omitted worker will try to guess it and will raised an error if it could not.",


Suggested change

"The application that provides the event. If omitted worker will try to guess it and will raised an error if it could not.",

"The application that provides the event. If omitted, worker will try to guess it and will raise an error if it cannot.",

RemiBou · 2024-11-27T09:13:54Z

commands/common/cmd_api.go

+	timeout, err := model.GetTimeoutParameter(c)
+	if err != nil {
+		return apiError(http.StatusInternalServerError, "%+v", err)
+	}


isnt it confusing to return an http error when we didnt call the server ?

The status is for internal use only.
Only err.Error() meaning the message is showed to the end user.

RemiBou · 2024-11-27T09:14:45Z

commands/common/cmd_api.go

+
+	apiEndpoint := fmt.Sprintf("%sworker/api/v%d/%s", utils.AddTrailingSlashIfNeeded(params.ServerUrl), params.ApiVersion+1, strings.Join(params.Path, "/"))
+
+	if params.Query != nil || params.ProjectKey != "" {


there isnt a built in way to generate query params ? with url apis

RemiBou · 2024-11-27T09:15:07Z

commands/common/cmd_api.go

+		}
+	}
+
+	reqCtx, cancelReq := context.WithTimeout(context.Background(), timeout)


maybe the context should be a parameter

The context is only used for request timeout.
I want to avoid duplicating context creation and cancellation on all the command.

ehl-jf requested a deployment to frogbot November 26, 2024 15:55 — with GitHub Actions Waiting

cyrilc-pro approved these changes Nov 27, 2024

View reviewed changes

RemiBou reviewed Nov 27, 2024

View reviewed changes

WKS-1193 - Use dynamic actions

a38304d

ehl-jf force-pushed the use-dynamic-actions branch from 7ac397e to a38304d Compare November 28, 2024 11:30

ehl-jf requested a deployment to frogbot November 28, 2024 11:31 — with GitHub Actions Waiting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WKS-1193 - Use dynamic actions #10

WKS-1193 - Use dynamic actions #10

ehl-jf commented Nov 26, 2024 •

edited

Loading

cyrilc-pro Nov 26, 2024

ehl-jf Nov 28, 2024

cyrilc-pro Nov 26, 2024

ehl-jf Nov 28, 2024

cyrilc-pro Nov 27, 2024

ehl-jf Nov 28, 2024 •

edited

Loading

cyrilc-pro Nov 28, 2024

cyrilc-pro Nov 27, 2024

cyrilc-pro Nov 27, 2024

cyrilc-pro Nov 27, 2024

ehl-jf Nov 28, 2024

cyrilc-pro Nov 27, 2024

ehl-jf Nov 28, 2024

cyrilc-pro Nov 27, 2024

ehl-jf Nov 28, 2024

cyrilc-pro Nov 27, 2024

ehl-jf Nov 28, 2024

cyrilc-pro Nov 27, 2024

RemiBou Nov 27, 2024

ehl-jf Nov 28, 2024

RemiBou Nov 27, 2024

RemiBou Nov 27, 2024

ehl-jf Nov 28, 2024


		defer CloseQuietly(res.Body)

		if res.ContentLength > 0 \|\| res.ContentLength == -1 {

-	if res.ContentLength > 0 || res.ContentLength == -1 {
+	if res.ContentLength == 0 {
+	// discard
+	} else {
+	//consume
+	}

	tsUnexportedType = regexp.MustCompile(`^(class\|type\|interface\|enum\|const)\s+[A-Za-z_$][0-9A-Za-z_$]*`)
	tsUnexportedType = regexp.MustCompile(`^\s(class\|type\|interface\|enum\|const)\s+[A-Za-z_$][0-9A-Za-z_$]`)

	return common.Print("%s", strings.Join(actions, ", "))
	return common.Print(strings.Join(actions, ", "))

	"The application that provides the event. If omitted worker will try to guess it and will raised an error if it could not.",
	"The application that provides the event. If omitted, worker will try to guess it and will raise an error if it cannot.",


		apiEndpoint := fmt.Sprintf("%sworker/api/v%d/%s", utils.AddTrailingSlashIfNeeded(params.ServerUrl), params.ApiVersion+1, strings.Join(params.Path, "/"))

		if params.Query != nil \|\| params.ProjectKey != "" {

WKS-1193 - Use dynamic actions #10

Are you sure you want to change the base?

WKS-1193 - Use dynamic actions #10

Conversation

ehl-jf commented Nov 26, 2024 • edited Loading

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

ehl-jf Nov 28, 2024 • edited Loading

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

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

ehl-jf commented Nov 26, 2024 •

edited

Loading

ehl-jf Nov 28, 2024 •

edited

Loading