-
Notifications
You must be signed in to change notification settings - Fork 23
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
API chaining POC #851
base: main
Are you sure you want to change the base?
API chaining POC #851
Conversation
c7ae5a1
to
f640c99
Compare
@@ -41,7 +45,18 @@ func (a ApiDomain) makeRequests(ctx context.Context) (types.DomainResources, err | |||
// requests with overrides (in request.Options.Headers) will get bespoke clients. | |||
defaultClient := clientFromOpts(defaultOpts) | |||
var errs error | |||
for _, request := range a.Spec.Requests { | |||
for i, request := range a.Spec.Requests { | |||
if i > 0 { // the first request cannot use outputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: but I've noticed this being heavily used in lula codebased, but something that makes me 😿
how about rewriting this like so:
for i, request := range a.Spec.Requests {
if i < 1 {
// the first request cannot use outputs
continue
}
if a.Spec.outputs != nil {
var err error
...
The benefit is that we're having less indented code which improves readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm onboard with the idea of reducing indentation, but that won't work in this example - here I'm not continuing out of the entire loop, just skipping the outputs processing (those first 10 lines).
request, err = executeTpls(request, a.Spec.outputs) | ||
if err != nil { | ||
/// TODO: better error | ||
return collection, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider if you want to process all the request and gather []error
and return that, or fail on the first error like you have here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the current design, and there's more (not yet implemented) nuance to the statement I'm about to make, but once we introduce templating we need to fail as soon as one API call fails, since the output of that call is the input of another. We cannot complete a chain of requests if a request in that chain fails.
I will say (here's the nuance) that I'd like refactor Lula so it has an idea of a request chain vs a single request, or some other way to identify while operations much happen first and succeed before the next can continue. This POC is meant to be the API chaining option that doesn't require a huge refactor, but I'm team refactor, myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also on my list of arguments for a DSL : the go-cty type system has Unknown and Dynamic type
placeholders, and I want that or something like it in Lula, so it's easier to look at a single API request and know that we're still waiting for placeholder values to be expanded. (we'd also need a dependency graph)(i know i'm just describing terraform, i'm sorry)
// | ||
// outputs is a map of stored outputs from API calls that can be used as | ||
// template variables for URLTpls | ||
outputs map[string]map[string]interface{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several ideas, take whatever you feel like makes sense:
- I'd not introduce this intermediary in the external types at all. Even though it's a private field, it still feels like exposing part of internal code. I'd suggest creating an internal structure, if you need to pass the data between one data structure and the other.
- I looked into the templating bits and I have similar concerns as you pointed out on slack, that in the long run this might cause pains. I might be missing some bits of information about the goal, but have you considered using jsonpath, which is already used in several places and can potentially provide a better escaping logic, and be easier to understand by users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is basically the syntax I have for users to define what fields they want from the previous API calls (i'm using the kyaml library, for consistency with other parts of lula).
That doesn't help me with the user input template part of things, though - since we're "templatizing" strings (parameters, headers, the URL) I do need some kind of string-based templating syntax?
303e4a7
to
c00a700
Compare
c00a700
to
87368f3
Compare
87368f3
to
a6634fe
Compare
POC: "templatizing" URL, headers, and parameters in the API domain with results from previous API calls.