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

Added Callback cli #446

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

Conversation

Madraceee
Copy link

@Madraceee Madraceee commented Oct 24, 2024

Description

Added callback cli. Invokes the inbuilt commands.

This is not finished

  1. What should the value of requestID be. UUID or some string?
  2. For url and default, how should those values be passed?

Sry I was busy with my work, could not look into what should be passed for callbacks which dont require a json

To-Do

  • Fix RequestID
  • Fix non json flow

@Madraceee
Copy link
Author

@dfarr I couldnt find an option to request for review , so can you help me out?

@dfarr dfarr self-requested a review October 24, 2024 19:19
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 100 lines in your changes missing coverage. Please review.

Project coverage is 54.91%. Comparing base (6b6ee59) to head (18216d1).

Files with missing lines Patch % Lines
cmd/callbacks/create.go 0.00% 72 Missing ⚠️
cmd/callbacks/callbacks.go 0.00% 28 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #446      +/-   ##
==========================================
- Coverage   55.25%   54.91%   -0.35%     
==========================================
  Files         119      121       +2     
  Lines       13753    13853     +100     
==========================================
+ Hits         7599     7607       +8     
- Misses       5690     5783      +93     
+ Partials      464      463       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dfarr dfarr left a comment

Choose a reason for hiding this comment

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

So sorry for the delay in getting back to you on this PR. The recv flag is so tricky because it can be either a string or a json object in the form {"type": "", data: {}}.

I played around a little with this, what do you think about this approach?

var recv v1.Recv

if json.Valid([]byte(recvStr)) {
	var recv0 v1.Recv0

	if err := json.Unmarshal([]byte(recvStr), &recv0); err != nil {
		return err
	}
	if err := recv.FromRecv0(recv0); err != nil {
		return err
	}
} else {
	if err := recv.FromRecv1(recvStr); err != nil {
		return err
	}
}

This unmarshals the provided value to a v1.Recv0 if the value provided by the user is valid json, otherwise we assume it's a string. That way the user can provide one or the other, for example:

# string example
go run ./... callbacks create --promise-id foo --root-promise-id bar --recv default

# json example
go run ./... callbacks create --promise-id foo --root-promise-id bar --recv '{"type":"poll", "data":{"group":"default","id":"0"}}'

RunE: func(cmd *cobra.Command, args []string) error {

// What should requestID be?
paramsByte, err := json.Marshal(struct {
Copy link
Member

Choose a reason for hiding this comment

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

We can remove requestId from the CLI, its an optional parameter that is more handy for web applications, you can pass nil to CreateCallbackWithResponse for the params.

},
}

cmd.Flags().StringVar(&promiseId, "promise-id", "", "promise id")
Copy link
Member

Choose a reason for hiding this comment

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

We can mark this one as required

}

cmd.Flags().StringVar(&promiseId, "promise-id", "", "promise id")
cmd.Flags().StringVar(&rootPromiseId, "root-promise-id", "", "root promise id")
Copy link
Member

Choose a reason for hiding this comment

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

We can also mark this one as required


cmd.Flags().StringVar(&promiseId, "promise-id", "", "promise id")
cmd.Flags().StringVar(&rootPromiseId, "root-promise-id", "", "root promise id")
cmd.Flags().DurationVar(&timeout, "timeout", 0, "callback timeout")
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 we can put a sensible default here, maybe 1m?

cmd.Flags().StringVar(&promiseId, "promise-id", "", "promise id")
cmd.Flags().StringVar(&rootPromiseId, "root-promise-id", "", "root promise id")
cmd.Flags().DurationVar(&timeout, "timeout", 0, "callback timeout")
cmd.Flags().StringVar(&recv, "recv", "", "receive object")
Copy link
Member

Choose a reason for hiding this comment

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

We can put default (literally) as the default for recv

@Madraceee
Copy link
Author

I am not well. I will look into this next week. Is that fine?

@dfarr
Copy link
Member

dfarr commented Nov 10, 2024

I am not well. I will look into this next week. Is that fine?

Of course, take your time and hope you get well soon!

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