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 HTTP Target #78

Merged
merged 8 commits into from
Nov 13, 2020
Merged

Add HTTP Target #78

merged 8 commits into from
Nov 13, 2020

Conversation

odacremolbap
Copy link
Member

@odacremolbap odacremolbap commented Nov 12, 2020

Add HTTP Target documentation.

closes #43

@odacremolbap odacremolbap self-assigned this Nov 12, 2020
Copy link
Contributor

@daceynolan daceynolan left a comment

Choose a reason for hiding this comment

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

Sorry for the nitpicky feedback


Responses from the remote endpoint will generate new [CloudEvents][ce] that will be returned to TriggerMesh. Most probably those response events should not be re-processed by the HTTP Target.

It is important that the Trigger that subscribes the HTTP Target to the Broker configure the appropiate filters to avoid these loops.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit picky - sorry appropiate here should beappropriate

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks
7bf9f2d


| Field | Description | Example |
|--- |--- |--- |
| query_string | Key/value pairs formated as query string | `name=jane&lastname=doe` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicky - formated here should be formatted

Copy link
Member Author

Choose a reason for hiding this comment

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

gracias!
7bf9f2d

- `CA Certificate` CA certificate configured for TLS connection.
- `Skip Verify` skips remote server TLS certificate verification.
- `Username` when using basic authentication.
- `Password` when using basic authentication.
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this require a secret?

Copy link
Member Author

Choose a reason for hiding this comment

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

it does, fixing!
thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

ありがとうございました
9a886ca


## Trigger Configuration

Responses from the remote endpoint will generate new [CloudEvents][ce] that will be returned to TriggerMesh. Most probably those response events should not be re-processed by the HTTP Target.
Copy link
Contributor

Choose a reason for hiding this comment

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

The Most probably part sounds confusing. You may need to mention to the user how to setup a trigger filter to ensure the http target doesn't end up reprocessing events.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right.
Most probably should be out.

I don't want to show here how to configure Triggers, we should have a generic dedicated page for that. Skipping for now and opening a follow up issue if that's ok with you.

Copy link
Contributor

Choose a reason for hiding this comment

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

どいたしまして!

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the confusing part.
634e10c

Waiting for your opinion on the trigger. I would like that one outside because transformation, infraJS, and every target that generates a response will be in the same situation. We might need a section for communication elements where we detail brokers and triggers for now, channels and subscriptions in the future.

@cab105
Copy link
Contributor

cab105 commented Nov 12, 2020

Would suggest perhaps a screenshot or two of the targets page, but otherwise looking good.

@odacremolbap
Copy link
Member Author

Would suggest perhaps a screenshot or two of the targets page, but otherwise looking good.

there is a reason I'm not adding screenshots ... I hope we move secrets somewhere else, or group them with passwords and related fields.

I hope we discuss that soon.

Copy link
Contributor

@cab105 cab105 left a comment

Choose a reason for hiding this comment

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

I like what you did. Left a few more minor nits, and am approving this for when you get it taken care of.

- `Password` when using basic authentication needs to reference the aforementioned password secret.
- `Headers` is a set of key/value pairs that will be set withing the HTTP request.

Save the target, fill the rest of the bridge components and press `Submit Bridge`.
Copy link
Contributor

Choose a reason for hiding this comment

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

...components, and press...

Copy link
Member Author

Choose a reason for hiding this comment

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

- HTTP Target is interested in events whose type is `calendar.pto.request`.
- The response from workday will generate a CloudEvent type `workday.pto.response` and source `workday.instance1`

Trigger should be configured to avoid feeding these responses into the HTTP Target, a filter key `type` and value `calendar.pto.request` would provide such protection
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

...responses into the HTTP Target. A filter key type and value calendar.pto.request would provide such protection.

Copy link
Member Author

Choose a reason for hiding this comment

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


The HTTP Target expects a cloud event request that complements the Target configured values.

There is no requirement regarding the type header value, any cloud event containing the expected data is valid to process. Data needs to be a JSON structure that might contain these optional fields:
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "...header value. Any cloud event..."

Copy link
Member Author

Choose a reason for hiding this comment

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



| Field | Description | Example |
|--- |--- |--- |
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need trailing spaces as mkdocs will format it correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@sameersbn sameersbn left a comment

Choose a reason for hiding this comment

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

minor nitpicks 😛

docs/targets/http.md Outdated Show resolved Hide resolved
docs/targets/http.md Outdated Show resolved Hide resolved

The HTTP event target sends requests to arbitrary URLs and wraps responses in CloudEvents back to the caller. HTTP endpoints that are unauthenticated, use basic authentication or use custom header values for authentication, can be integrated using this target.

Responses from external HTTP endpoints are converted into [CloudEvents][ce] and sent as a reply to the TriggerMesh Broker/Channel. It is important that the HTTP target filters received events and cares about response event type and event source to avoid loops where those responses might end up being processed by the HTTP Target.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need to link to CloudEvents page every time it;s mentioned?

Copy link
Member Author

Choose a reason for hiding this comment

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

probably not, I'll set it only once at the first mention and remove all other links if that's ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

{"body": "{\"records\":[{\"value\":{\"this\":{\"is\": \"sparta\"}}}]}"}
```


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra new-line

Copy link
Member Author

Choose a reason for hiding this comment

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

odacremolbap and others added 3 commits November 13, 2020 10:42
Co-authored-by: Sameer Naik <sameer@triggermesh.com>
Co-authored-by: Sameer Naik <sameer@triggermesh.com>
@sameersbn
Copy link
Contributor

lgtm! approved

@odacremolbap odacremolbap merged commit 19186c9 into master Nov 13, 2020
@odacremolbap odacremolbap deleted the task/add-http-target branch November 13, 2020 09:52
@antoineco antoineco mentioned this pull request Apr 15, 2021
36 tasks
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.

HTTP Target: add docs
4 participants