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

Add --header/-H to dapr invoke #1287

Closed
wants to merge 15 commits into from
Closed

Conversation

hunter007
Copy link

@hunter007 hunter007 commented May 20, 2023

Description

to support pass customized headers to dapr app

See dapr/go-sdk#405

Issue reference

Fix #1286

Copy link
Collaborator

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

@hunter007 Thanks for adding this feature.
Can you add E2E for the this feature please?

Signed-off-by: hunter007 <[email protected]>
@hunter007
Copy link
Author

@hunter007 Thanks for adding this feature. Can you add E2E for the this feature please?

:) Review this PR first: dapr/go-sdk#405

@mukundansundar
Copy link
Collaborator

@hunter007 Thanks for adding this feature. Can you add E2E for the this feature please?

:) Review this PR first: dapr/go-sdk#405

seems the PR has been merged :)

@hunter007
Copy link
Author

@hunter007 Thanks for adding this feature. Can you add E2E for the this feature please?

:) Review this PR first: dapr/go-sdk#405

seems the PR has been merged :)

@hunter007 Thanks for adding this feature. Can you add E2E for the this feature please?

:) Review this PR first: dapr/go-sdk#405

seems the PR has been merged :)

Yes, so the PR could be reviewd.

@mukundansundar
Copy link
Collaborator

@hunter007 Thanks for adding this feature. Can you add E2E for the this feature please?

:) Review this PR first: dapr/go-sdk#405

seems the PR has been merged :)

@hunter007 Thanks for adding this feature. Can you add E2E for the this feature please?

:) Review this PR first: dapr/go-sdk#405

seems the PR has been merged :)

Yes, so the PR could be reviewd.

Won't the dependency need to be updated to point to the latest go-sdk in this case?

@hunter007
Copy link
Author

@hunter007 Thanks for adding this feature. Can you add E2E for the this feature please?

:) Review this PR first: dapr/go-sdk#405

seems the PR has been merged :)

@hunter007 Thanks for adding this feature. Can you add E2E for the this feature please?

:) Review this PR first: dapr/go-sdk#405

seems the PR has been merged :)

Yes, so the PR could be reviewd.

Won't the dependency need to be updated to point to the latest go-sdk in this case?

no need.

@hunter007 Thanks for adding this feature. Can you add E2E for the this feature please?

:) Review this PR first: dapr/go-sdk#405

seems the PR has been merged :)

@hunter007 Thanks for adding this feature. Can you add E2E for the this feature please?

:) Review this PR first: dapr/go-sdk#405

seems the PR has been merged :)

Yes, so the PR could be reviewd.

Won't the dependency need to be updated to point to the latest go-sdk in this case?

No need. dapr/go-sdk#405 is used when writing app with go-sdk. Without dapr/go-sdk#405, cli invoke will still send HTTP header by -H to dapr app

@mukundansundar
Copy link
Collaborator

@hunter007 the specific test that you have written seems to fail output does not contain "aValue" ... Can you take a look into this?

@hunter007
Copy link
Author

@hunter007 the specific test that you have written seems to fail output does not contain "aValue" ... Can you take a look into this?

It dependent on dapr/go-sdk which's version is after v1.7.0.

@mukundansundar
Copy link
Collaborator

@hunter007 the specific test that you have written seems to fail output does not contain "aValue" ... Can you take a look into this?

It dependent on dapr/go-sdk which's version is after v1.7.0.

This PR needs to be modified to use the latest go-sdk then in tests.

@mukundansundar
Copy link
Collaborator

@hunter007 1.8 version of go-sdk was released and I believe that needs to be used for the header functionality to work.

@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Merging #1287 (83d8a3a) into master (31b9ea2) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1287      +/-   ##
==========================================
+ Coverage   26.81%   26.87%   +0.05%     
==========================================
  Files          39       39              
  Lines        3882     3885       +3     
==========================================
+ Hits         1041     1044       +3     
  Misses       2767     2767              
  Partials       74       74              
Impacted Files Coverage Δ
pkg/standalone/client.go 0.00% <ø> (ø)
pkg/standalone/invoke.go 73.33% <100.00%> (+1.90%) ⬆️

@hunter007
Copy link
Author

@mukundansundar github action timeouted, have a retry?

Copy link
Collaborator

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

small changes please.

cmd/invoke.go Outdated
@@ -98,6 +121,7 @@ func init() {
InvokeCmd.Flags().StringVarP(&invokeData, "data", "d", "", "The JSON serialized data string (optional)")
InvokeCmd.Flags().StringVarP(&invokeVerb, "verb", "v", defaultHTTPVerb, "The HTTP verb to use")
InvokeCmd.Flags().StringVarP(&invokeDataFile, "data-file", "f", "", "A file containing the JSON serialized data (optional)")
InvokeCmd.Flags().StringArrayVarP(&invokeHeaders, "header", "H", []string{}, "0 or more HTTP Header")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
InvokeCmd.Flags().StringArrayVarP(&invokeHeaders, "header", "H", []string{}, "0 or more HTTP Header")
InvokeCmd.Flags().StringArrayVarP(&invokeHeaders, "header", "H", []string{}, "HTTP headers to be used on invoke")

cmd/invoke.go Show resolved Hide resolved
resp: "successful invoke",
},
{
name: "appID found successful invoke with customized header",
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add one test with multiple header values.

Signed-off-by: hunter007 <[email protected]>
@hunter007 hunter007 closed this Jul 25, 2023
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.

Support HTTP Header for dapr invoke
3 participants