-
Notifications
You must be signed in to change notification settings - Fork 37
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 created, updated, and provisioned timestamps to saved template #551
Conversation
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #551 +/- ##
============================================
- Coverage 72.98% 72.81% -0.18%
- Complexity 662 675 +13
============================================
Files 79 79
Lines 3395 3451 +56
Branches 264 273 +9
============================================
+ Hits 2478 2513 +35
- Misses 803 822 +19
- Partials 114 116 +2 ☔ View full report in Codecov by Sentry. |
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.
Lgtm, pending fix for failing integration tests. Just a small comment
src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java
Outdated
Show resolved
Hide resolved
Yeah, I chose to update the state index before updating the template in the global context which triggers this. I can easily reverse the order (it made more sense that way) but I wanted to flip the flag to provisioning as soon as possible to avoid a race condition! Will see if there's another workaround. |
Signed-off-by: Daniel Widdis <[email protected]>
9d1089d
to
19156ec
Compare
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
POST localhost:9200/_plugins/_flow_framework/workflow/
{
"name": "test",
"description": "Flow template",
"use_case": "TESTING",
"version": {
"template": "1.0.0",
"compatibility": ["2.12.0", "3.0.0"]
},
"workflows": {
"provision": {
"user_params": {},
"nodes": [
{
"id": "test-step",
"type": "noop"
}
]
}
}
} {
"workflow_id": "v5icD44BN3l14GXsicR6"
} GET localhost:9200/_plugins/_flow_framework/workflow/v5icD44BN3l14GXsicR6 {
"name": "test",
"description": "Flow template",
"use_case": "TESTING",
"version": {
"template": "1.0.0",
"compatibility": [
"2.12.0",
"3.0.0"
]
},
"workflows": {
"provision": {
"user_params": {},
"nodes": [
{
"id": "test-step",
"type": "noop",
"previous_node_inputs": {},
"user_inputs": {}
}
],
"edges": []
}
},
"created_time": 1709658900798,
"last_updated_time": 1709658900798
} PUT localhost:9200/_plugins/_flow_framework/workflow/v5icD44BN3l14GXsicR6
{
"name": "test-update",
"description": "Flow template",
"use_case": "TESTING",
"version": {
"template": "1.0.0",
"compatibility": ["2.12.0", "3.0.0"]
},
"workflows": {
"provision": {
"user_params": {},
"nodes": [
{
"id": "test-step",
"type": "noop"
}
]
}
}
} {
"workflow_id": "v5icD44BN3l14GXsicR6"
} GET localhost:9200/_plugins/_flow_framework/workflow/v5icD44BN3l14GXsicR6 {
"name": "test-update",
"description": "Flow template",
"use_case": "TESTING",
"version": {
"template": "1.0.0",
"compatibility": [
"2.12.0",
"3.0.0"
]
},
"workflows": {
"provision": {
"user_params": {},
"nodes": [
{
"id": "test-step",
"type": "noop",
"previous_node_inputs": {},
"user_inputs": {}
}
],
"edges": []
}
},
"created_time": 1709658900798,
"last_updated_time": 1709658977120
} POST localhost:9200/_plugins/_flow_framework/workflow/v5icD44BN3l14GXsicR6/_provision {
"workflow_id": "v5icD44BN3l14GXsicR6"
} GET localhost:9200/_plugins/_flow_framework/workflow/v5icD44BN3l14GXsicR6 {
"name": "test-update",
"description": "Flow template",
"use_case": "TESTING",
"version": {
"template": "1.0.0",
"compatibility": [
"2.12.0",
"3.0.0"
]
},
"workflows": {
"provision": {
"user_params": {},
"nodes": [
{
"id": "test-step",
"type": "noop",
"previous_node_inputs": {},
"user_inputs": {}
}
],
"edges": []
}
},
"created_time": 1709658900798,
"last_updated_time": 1709658977120,
"last_provisioned_time": 1709659053752
} |
Start and end time should already be in the state index |
Yes, they are. This should match (within milliseconds) the start time of the latest provisioning attempt. Probably didn't need to add it here, but now that it's here should we leave it or take it back out? |
14fcb40
to
19772b7
Compare
873bf6a
to
a9b0a51
Compare
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
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.
Test classes yet to be reviewed. Added few comments
src/main/java/org/opensearch/flowframework/common/CommonValue.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/flowframework/bwc/FlowFrameworkBackwardsCompatibilityIT.java
Show resolved
Hide resolved
Signed-off-by: Daniel Widdis <[email protected]>
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.
Overall looks good. Few comments
src/test/java/org/opensearch/flowframework/bwc/FlowFrameworkBackwardsCompatibilityIT.java
Show resolved
Hide resolved
src/test/java/org/opensearch/flowframework/bwc/FlowFrameworkBackwardsCompatibilityIT.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/flowframework/bwc/FlowFrameworkBackwardsCompatibilityIT.java
Show resolved
Hide resolved
Signed-off-by: Daniel Widdis <[email protected]>
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 can improve on the manual copying of distribution to zip in the future
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.
LGTM! Thanks for addressing the comments.
) * Add created, last updated, and last provisioned fields to Template Signed-off-by: Daniel Widdis <[email protected]> * Change last updated timestamp when updating workflow Signed-off-by: Daniel Widdis <[email protected]> * Change last provisioned timestamp when provisioning workflow Signed-off-by: Daniel Widdis <[email protected]> * Allow overriding template not started check Signed-off-by: Daniel Widdis <[email protected]> * Use java.time and not joda time Signed-off-by: Daniel Widdis <[email protected]> * Preserve timestamps when encrypting and redacting template Signed-off-by: Daniel Widdis <[email protected]> * Add bwc tests, more timestamp testing Signed-off-by: Daniel Widdis <[email protected]> * Build a Template from an existing one Signed-off-by: Daniel Widdis <[email protected]> * Rename param, add comments Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> (cherry picked from commit 2707210) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ved template (#556) * Add created, updated, and provisioned timestamps to saved template (#551) * Add created, last updated, and last provisioned fields to Template Signed-off-by: Daniel Widdis <[email protected]> * Change last updated timestamp when updating workflow Signed-off-by: Daniel Widdis <[email protected]> * Change last provisioned timestamp when provisioning workflow Signed-off-by: Daniel Widdis <[email protected]> * Allow overriding template not started check Signed-off-by: Daniel Widdis <[email protected]> * Use java.time and not joda time Signed-off-by: Daniel Widdis <[email protected]> * Preserve timestamps when encrypting and redacting template Signed-off-by: Daniel Widdis <[email protected]> * Add bwc tests, more timestamp testing Signed-off-by: Daniel Widdis <[email protected]> * Build a Template from an existing one Signed-off-by: Daniel Widdis <[email protected]> * Rename param, add comments Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> (cherry picked from commit 2707210) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Update imports for 2.x branch Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Daniel Widdis <[email protected]>
Description
Adds created time, last updated time, and last provisioned time to the template stored in the global context.
Also adds BWC tests!
Also fixes Integ tests which were essentially running twice since yamlRestTest was not properly filtered and was running them a second time.
Issues Resolved
Fixes #548
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.