-
Notifications
You must be signed in to change notification settings - Fork 27
feat(specs): add external injection source definition and runtime parameter #5283
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
Conversation
✔️ Code generated!
📊 Benchmark resultsBenchmarks performed on the method using a mock server, the results might not reflect the real-world performance.
|
3c21967 to
61a020b
Compare
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.
Small comment but first shot is super clean 👍
specs/composition-full/common/schemas/components/InjectionSource.yml
Outdated
Show resolved
Hide resolved
specs/composition-full/common/schemas/components/InjectionSource.yml
Outdated
Show resolved
Hide resolved
d443f32 to
d58c58f
Compare
| A list of injected objectIDs into an external source. | ||
| default: [] | ||
| example: | ||
| [{'objectID': 'my-object-1', 'metadata': {'my-field': 'my-value'}}] |
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.
Have you verified the generated examples look correct? Typically JSON requires " instead of '
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.
Not sure how to generate examples
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.
we don't generate examples or tests from the specs, any test created in the CTS is then exposed a fully validated code snippet that can be exposed on the docs
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.
you can find the output here #5283 (comment) when the CI is done running, under the docs/snippets folder
| required: | ||
| - search | ||
| description: Search source to be used to inject items into result set. | ||
| $ref: '#/injectionSource' |
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: the previous name made it clear the type was only good for injectedItems. I think you could even keep it inline here since it can't be reused outside injectedItems
| $ref: '#/injectionSource' | |
| title: injectedItemSource | |
| oneOf: | |
| - $ref: './InjectionSource.yml#/search' | |
| - $ref: './InjectionSource.yml#/external' |
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.
Changed the name, but had to keep 'oneOf' statement as root component. It cannot be nested, as I'm getting an error:
56:5 error oneOf must be out of line automation-custom/out-of-line-one-of
| - type: object | ||
| additionalProperties: false |
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.
Do you need this object w/ no properties?
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.
Dropped this
| objectID: | ||
| type: string | ||
| metadata: | ||
| $ref: '../schemas/components/CompositionBehavior.yml#/metadata' |
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 points to the entire metadata object, but it should really just be generic key-value pairs. I think technically something like this might work, but there's probably a better way
$ref: '../schemas/components/CompositionBehavior.yml#/metadata/properties/hits/properties/extra'
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.
Removed reference
specs/composition-full/common/schemas/components/InjectionSource.yml
Outdated
Show resolved
Hide resolved
866333f to
5c5302b
Compare
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.
Small comment but LGTM otherwise
dc532ae to
d1d482f
Compare
|
Hello, there is an issue with the CTS, I think you need to update the |
I've fixed this, the error was a little deceptive because the issue was with injectedItemSource in CompositionBehavior.json. However, when I generate typescript locally, I don't quite see the result that I should expect. For example, in external.ts import type { Search } from './search';
/**
* Injected items will originate from externally provided objectIDs (that must exist in the index) given at runtime in the run request payload.
*/
export type External = Search;The |
|
Unfortunately the generator does not support anonymous objects, because it's not possible to generate in all the languages that we support. |
That worked! Thank you. |
94b9ff7 to
e0441d1
Compare
Co-authored-by: Clara Muller <[email protected]>
483c3dc to
9e65302
Compare
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.
thanks for the contribution !
🧭 What and Why
🎟 JIRA Ticket:
as part of
Changes included:
externalinjection search sourceinjectedItemscan now either be asearchorexternalsource.injectedItemscompositions runtime parameter.🧪 Test
CI