Support combining multiple output variables. by dblock · Pull Request #737 · opensearch-project/opensearch-api-specification

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

@@ -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

+  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

+  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

-    } 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

+      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 <dblock@amazon.com>

* Support combining multiple output variables. Signed-off-by: dblock <dblock@amazon.com>

* Support combining multiple output variables. Signed-off-by: dblock <dblock@amazon.com> Signed-off-by: Tokesh <tokesh789@gmail.com>

* adding missing tests Signed-off-by: Tokesh <tokesh789@gmail.com> * fixing validate ci, links, added node failure in specs Signed-off-by: Tokesh <tokesh789@gmail.com> * 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 <tokesh789@gmail.com> * Support combining multiple output variables. (#737) * Support combining multiple output variables. Signed-off-by: dblock <dblock@amazon.com> Signed-off-by: Tokesh <tokesh789@gmail.com> * hotfix of method in snapshots Signed-off-by: Tokesh <tokesh789@gmail.com> * hotfix with snapshot tests Signed-off-by: Tokesh <tokesh789@gmail.com> * hotfix race condition Signed-off-by: Tokesh <tokesh789@gmail.com> * adding chapter in snapshot tests Signed-off-by: Tokesh <tokesh789@gmail.com> * correcting path to repository in snapshot tests Signed-off-by: Tokesh <tokesh789@gmail.com> * adding verbose to check ci Signed-off-by: Tokesh <tokesh789@gmail.com> * deleting verbose Signed-off-by: Tokesh <tokesh789@gmail.com> * added retry to status in snapshots Signed-off-by: Tokesh <tokesh789@gmail.com> * added retry to correct place Signed-off-by: Tokesh <tokesh789@gmail.com> * adding verbose to check Signed-off-by: Tokesh <tokesh789@gmail.com> * renaming to avoid race condition Signed-off-by: Tokesh <tokesh789@gmail.com> * refactoring folder organization, adding retries, fixing naming Signed-off-by: Tokesh <tokesh789@gmail.com> --------- Signed-off-by: Tokesh <tokesh789@gmail.com> Signed-off-by: dblock <dblock@amazon.com> Signed-off-by: Niyazbek Torekeldi <78027392+Tokesh@users.noreply.github.com> Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

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 Tokesh, VachaShah, Xtansia, aabeshov, harshavamsi, nhtruong and sachetalva 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 <dblock@amazon.com>

Removed variable expansion from ML example.

ff45ab9

Signed-off-by: dblock <dblock@amazon.com>

Refactored split and OutputReference.

43dc5f2

Signed-off-by: dblock <dblock@amazon.com>

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

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 <dblock@amazon.com>

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

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

Conversation

dblock commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Uh oh!

github-actions Bot commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes Analysis

API Changes

Summary

Report

API Coverage

Uh oh!

github-actions Bot commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Spec Test Coverage Analysis

Uh oh!

dblock commented Dec 17, 2024

Uh oh!

nhtruong Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

nhtruong Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

nhtruong Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

dblock Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

nhtruong Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

dblock Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

nhtruong Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblock Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

dblock commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblock commented Dec 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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