-
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 allow_delete parameter to Deprovision API #763
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #763 +/- ##
============================================
+ Coverage 74.61% 75.41% +0.79%
- Complexity 783 816 +33
============================================
Files 84 88 +4
Lines 3873 3998 +125
Branches 356 362 +6
============================================
+ Hits 2890 3015 +125
Misses 825 825
Partials 158 158 ☔ View full report in Codecov by Sentry. |
4b3aa7a
to
ca7aed8
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.
At a high level LGTM, thanks for adding this! Meets the frontend requirements and will make deprovisioning/reprovisioning a much smoother experience. I concur with a 403 response on resources left over, I think that makes the most sense.
src/main/java/org/opensearch/flowframework/common/WorkflowResources.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/DeprovisionWorkflowTransportAction.java
Show resolved
Hide resolved
fc61d46
to
2de5517
Compare
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
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.
Looks good overall with one comment
src/main/java/org/opensearch/flowframework/workflow/DeleteSearchPipelineStep.java
Show resolved
Hide resolved
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/flow-framework/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/flow-framework/backport-2.x
# Create a new branch
git switch --create backport/backport-763-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7e7e99305d88b9f8ece719fd0b3b2183e8a6b82b
# Push it to GitHub
git push --set-upstream origin backport/backport-763-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/flow-framework/backport-2.x Then, create a pull request where the |
* Add allow_delete parameter to Deprovision API * Add denylist for workflow step types users are not allowed to use Signed-off-by: Daniel Widdis <[email protected]> * Remove denylisted steps from GetWorkflowStep response Signed-off-by: Daniel Widdis <[email protected]> * Remove mention of new steps from change log Signed-off-by: Daniel Widdis <[email protected]> --------- Signed-off-by: Daniel Widdis <[email protected]>
…785) Add allow_delete parameter to Deprovision API (#763) * Add allow_delete parameter to Deprovision API * Add denylist for workflow step types users are not allowed to use * Remove denylisted steps from GetWorkflowStep response * Remove mention of new steps from change log --------- Signed-off-by: Daniel Widdis <[email protected]>
Description
Adds an
allow_delete
parameter to the Deprovision Workflow API which can contain a comma-delimited list of resource ids which may be deleted.When the parameter does not contain resources that require it, the deprovision API returns a 403 response identifying the remaining resources.
Added
DeleteIndexStep
,DeleteIngestPipelineStep
, andDeleteSearchPipelineStep
which require the use of this parameter.Added a denylist to prevent the use of these steps when provisioning workflows, and filtered the Get Workflow Steps API return to remove them from that list as well
Issues Resolved
Documentation
Please review:
Note to Reviewers
This implementation causes a change in API behavior.
Current behavior is to simply remove these unimplemented deletions from the resource list and silently fail, giving a 200 (OK) on successful deprovision while the resources remain. (As a concrete example, if an index was created and included in the created resources list, deprovisioning would simply remove it from the resources list without taking any action (deleting the index).)
This is misleading, as an attempt to provision again will fail with a pre-existing index, so I think the change is justified, but should be made clear to anyone using existing scripting relying on this behavior.
I think 403 (Forbidden) is the best response type here, but I'm open to discussion here if you have a better proposal.
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.