Support combining multiple output variables. #737

dblock · 2024-12-16T17:23:12Z

Description

Support combining multiple output variables, e.g. task_id: ${task.node}:${task.id}. I ended up not using this and getting the task ID from delete_by_query, but it's a useful feature IMO.

Issues Resolved

Part of #663.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2024-12-16T17:25:24Z

Changes Analysis

Commit SHA: 43dc5f2
Comparing To SHA: 1b6935f

API Changes

Summary

NO CHANGES

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/12397608942/artifacts/2338846969

API Coverage

	Before	After	Δ
Covered (%)	606 (59.35 %)	606 (59.35 %)	0 (0 %)
Uncovered (%)	415 (40.65 %)	415 (40.65 %)	0 (0 %)
Unknown	43	43	0

github-actions · 2024-12-16T18:32:58Z

Spec Test Coverage Analysis

Total	Tested
536	536 (100 %)

dblock · 2024-12-17T18:06:53Z

@nhtruong care to take a look pls?

nhtruong · 2024-12-17T19:18:08Z

tools/src/tester/types/eval.types.ts

@@ -87,16 +87,42 @@ export enum Result {
 export class OutputReference {


I missed this when this was first added but this file is meant to hold type definitions only. This class should be in its own file.

nhtruong · 2024-12-17T19:24:54Z

tools/src/tester/types/eval.types.ts

+  static replace(str: string, callback: (chapter_id: any, variable: any) => string): any {
+    // preserve type if 1 value is returned
+    let full_match = str.match(/^\$\{([^}]+)\}$/)
+    if (full_match) {
+      const spl = this.#split(full_match[1], '.', 2)
+      return callback(spl[0], spl[1])
+    } else return str.replace(/\$\{([^}]+)\}/g, (_, k) => {
+      const spl = this.#split(k, '.', 2)
+      return callback(spl[0], spl[1])
+    });
+  }


Should add a comment explaining what this function does because it's not clear what it's trying to achieve.

nhtruong · 2024-12-17T19:25:54Z

tools/src/tester/types/eval.types.ts

+  static #split(str: any, delim: string, count: number): string[] {
+    if (str === undefined) return [str]
+    if (count <= 0) return [str]
+    const parts = str.split(delim)
+    if (parts.length <= count) return parts
+    const result = parts.slice(0, count - 1)
+    result.push(parts.slice(count - 1).join(delim))
+    return result


This needs an explanation as well.

Moved to helpers.

nhtruong · 2024-12-17T19:39:30Z

tools/src/tester/StoryOutputs.ts

-    } else {
-      return str
-    }
+    return OutputReference.replace(str, (chapter_id, output_name) => {


If this is the only place the replace function is used, then having a generic replace function that can handle any callback is a bit over-engineered, IMO.

I think it's best we keep the parsing regex values/logic in one place so it doesn't leak into the usage of it.

nhtruong · 2024-12-17T19:42:22Z

tools/tests/types/eval.types.test.ts

+      expect(OutputReference.replace('string', f)).toEqual('string')
+      expect(OutputReference.replace('${k.v}', f)).toEqual('[k:v]')
+      expect(OutputReference.replace('${k.value}', f)).toEqual('[k:value]')
+      expect(OutputReference.replace('${k.v.m}', f)).toEqual('[k:v.m]')


I was on board until the last example. It's not clearly why k is treated differently from v and m and how the callback f caused such an output.

Our output variables are split with namespace.xyz where namespace=payload and is meaningful, but the rest of it is not and can be anything, including something that contains periods. That's basically what this is testing.

dblock · 2024-12-17T20:15:06Z

~~Converted this to draft, will iterate. Extracted unrelated tests to #746.~~

dblock · 2024-12-18T16:13:10Z

Ready to be reviewed again, thanks @nhtruong!

Signed-off-by: dblock <[email protected]>

* Support combining multiple output variables. Signed-off-by: dblock <[email protected]>

* Support combining multiple output variables. Signed-off-by: dblock <[email protected]> Signed-off-by: Tokesh <[email protected]>

* adding missing tests Signed-off-by: Tokesh <[email protected]> * fixing validate ci, links, added node failure in specs Signed-off-by: Tokesh <[email protected]> * adding node failures to changelog, fixing path in specs of update by query and small fix in response of query group tests Signed-off-by: Tokesh <[email protected]> * Support combining multiple output variables. (#737) * Support combining multiple output variables. Signed-off-by: dblock <[email protected]> Signed-off-by: Tokesh <[email protected]> * hotfix of method in snapshots Signed-off-by: Tokesh <[email protected]> * hotfix with snapshot tests Signed-off-by: Tokesh <[email protected]> * hotfix race condition Signed-off-by: Tokesh <[email protected]> * adding chapter in snapshot tests Signed-off-by: Tokesh <[email protected]> * correcting path to repository in snapshot tests Signed-off-by: Tokesh <[email protected]> * adding verbose to check ci Signed-off-by: Tokesh <[email protected]> * deleting verbose Signed-off-by: Tokesh <[email protected]> * added retry to status in snapshots Signed-off-by: Tokesh <[email protected]> * added retry to correct place Signed-off-by: Tokesh <[email protected]> * adding verbose to check Signed-off-by: Tokesh <[email protected]> * renaming to avoid race condition Signed-off-by: Tokesh <[email protected]> * refactoring folder organization, adding retries, fixing naming Signed-off-by: Tokesh <[email protected]> --------- Signed-off-by: Tokesh <[email protected]> Signed-off-by: dblock <[email protected]> Signed-off-by: Niyazbek Torekeldi <[email protected]> Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>

dblock force-pushed the tasks-vars branch 5 times, most recently from 2990ceb to 3b06ff0 Compare December 16, 2024 18:32

dblock force-pushed the tasks-vars branch from 3b06ff0 to d9c0631 Compare December 16, 2024 21:58

dblock changed the title ~~Support combining multiple output variables.~~ Support combining multiple output variables, added tests for tasks. Dec 16, 2024

dblock marked this pull request as ready for review December 16, 2024 22:24

dblock requested review from harshavamsi, sachetalva, nhtruong, Xtansia, VachaShah, Tokesh and aabeshov as code owners December 16, 2024 22:24

nhtruong reviewed Dec 17, 2024

View reviewed changes

dblock mentioned this pull request Dec 17, 2024

Added missing task tests. #746

Merged

dblock marked this pull request as draft December 17, 2024 20:14

dblock force-pushed the tasks-vars branch 2 times, most recently from f632b2a to c546f0e Compare December 18, 2024 15:06

dblock marked this pull request as ready for review December 18, 2024 15:07

dblock requested a review from nhtruong December 18, 2024 15:07

dblock changed the title ~~Support combining multiple output variables, added tests for tasks.~~ Support combining multiple output variables. Dec 18, 2024

dblock added 2 commits December 18, 2024 11:52

Support combining multiple output variables.

6a9a90e

Signed-off-by: dblock <[email protected]>

Removed variable expansion from ML example.

ff45ab9

Signed-off-by: dblock <[email protected]>

Refactored split and OutputReference.

43dc5f2

Signed-off-by: dblock <[email protected]>

dblock force-pushed the tasks-vars branch from c546f0e to 43dc5f2 Compare December 18, 2024 16:53

nhtruong approved these changes Dec 18, 2024

View reviewed changes

nhtruong merged commit 4231dad into opensearch-project:main Dec 18, 2024
29 of 30 checks passed

Tokesh pushed a commit to Tokesh/opensearch-api-specification that referenced this pull request Dec 18, 2024

Support combining multiple output variables. (opensearch-project#737)

6c4647c

* Support combining multiple output variables. Signed-off-by: dblock <[email protected]>

dblock deleted the tasks-vars branch December 18, 2024 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support combining multiple output variables. #737

Support combining multiple output variables. #737

dblock commented Dec 16, 2024 •

edited

Loading

github-actions bot commented Dec 16, 2024 •

edited

Loading

github-actions bot commented Dec 16, 2024 •

edited

Loading

dblock commented Dec 17, 2024

nhtruong Dec 17, 2024

nhtruong Dec 17, 2024

nhtruong Dec 17, 2024

dblock Dec 18, 2024

nhtruong Dec 17, 2024

dblock Dec 18, 2024

nhtruong Dec 17, 2024 •

edited

Loading

dblock Dec 18, 2024

dblock commented Dec 17, 2024 •

edited

Loading

dblock commented Dec 18, 2024

		@@ -87,16 +87,42 @@ export enum Result {
		export class OutputReference {

Support combining multiple output variables. #737

Support combining multiple output variables. #737

Conversation

dblock commented Dec 16, 2024 • edited Loading

Description

Issues Resolved

github-actions bot commented Dec 16, 2024 • edited Loading

Changes Analysis

API Changes

Summary

Report

API Coverage

github-actions bot commented Dec 16, 2024 • edited Loading

Spec Test Coverage Analysis

dblock commented Dec 17, 2024

nhtruong Dec 17, 2024

Choose a reason for hiding this comment

nhtruong Dec 17, 2024

Choose a reason for hiding this comment

nhtruong Dec 17, 2024

Choose a reason for hiding this comment

dblock Dec 18, 2024

Choose a reason for hiding this comment

nhtruong Dec 17, 2024

Choose a reason for hiding this comment

dblock Dec 18, 2024

Choose a reason for hiding this comment

nhtruong Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

dblock Dec 18, 2024

Choose a reason for hiding this comment

dblock commented Dec 17, 2024 • edited Loading

dblock commented Dec 18, 2024

dblock commented Dec 16, 2024 •

edited

Loading

github-actions bot commented Dec 16, 2024 •

edited

Loading

github-actions bot commented Dec 16, 2024 •

edited

Loading

nhtruong Dec 17, 2024 •

edited

Loading

dblock commented Dec 17, 2024 •

edited

Loading