Skip to content
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

st2 API and CLI command added for actions/workflows clone operation #5345

Merged
merged 48 commits into from
Dec 1, 2021

Conversation

mahesh-orch
Copy link
Contributor

This PR adds st2 Clone API for actions and workflows.

  1. API clones action from source pack to destination pack and renames them appropriately.
  2. Also, this operation makes changes in destination action metadata files like action name, pack name, entry point name, etc. Action clone API takes source_pack_name, source_action_name, destination_pack_name, and destination_action_name in the endpoint.
  3. The clone operation registers the new action/workflow to the database. The clone operation throws exceptions appropriately (i.e requested source does not exist, destination pack does not exist, destination action already exists unless overwrite is specified, a user does not have permission, etc.)
  4. The clone operation takes an optional parameter in the query (?overwrite=True) to force the clone (or overwrite if the destination already exists).
  5. CLI command added for cloning operation. It takes four positional arguments source_pack_name, source_action_name, destination_pack_name and destination_action_name. Also, it takes an optional argument (-f or --force) to overwrite action file content if the destination already exists.

@pull-request-size pull-request-size bot added the size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. label Aug 27, 2021
@m4dcoder m4dcoder added this to the 3.6.0 milestone Aug 27, 2021
@CLAassistant
Copy link

CLAassistant commented Sep 6, 2021

CLA assistant check
All committers have signed the CLA.

@pull-request-size pull-request-size bot added size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several. and removed size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. labels Sep 14, 2021
@mahesh-orch
Copy link
Contributor Author

@m4dcoder

  • Permission type changed as ACTION_VIEW on source action, ACTION_CREATE on destination action. Added unit test for each scenario (/~https://github.com/ashwini-orchestral/st2/blob/clone_action_api/st2api/tests/unit/controllers/v1/test_actions_rbac.py)
  • Here for registering single cloned action there are two options:
    1. To copy the action the source_action_db instance to destination_action_db and modify it according to the user inputs and then use Action.add_or_update(destination_action_db) to register it to database. If destination already exists then de-register it before and then register a cloned_destination_action_db.
      (I have used the first way) Also, it is convenient here to check ACTION_CREATE permission on destination pack if destination action doesn't exist.
    2. To write a function in /st2common/bootstrap/actionsregistrar.py to register single action in a pack.
  • I didn't consolidate logic to single self.manager.clone because for args.force or user input "y" or "yes" overwrite flag will become True otherwise False and it is being used to prepare payload as body or clone post request.
  • As suggested, implemented ActionCloneCommand and returning NotImplementedError in ResourceCloneCommand for other resources.
  • Changed clone API endpoint to /api/v1/actions/{ref_or_id} and accepting other required and optional args in json body.
  • Added a function named remove_unnecessary_files_from_pack in /st2common/services/packs.py. While cloning an action, if there is a case of overwrite destination and the source runner type is different than the destination runner type then after cloning operation, unnecessary entry point files need to be removed from destination pack.
    I have added a separate function for this purpose.
  • Also, have made other minor changes as suggested.
    Please review the changes. Thanks.

@mahesh-orch
Copy link
Contributor Author

@m4dcoder

  • I have refactored the clone action function as per your suggestions and refactored corresponding unit tests.
  • I kept clone_action_files and clone_action_db functions separate, because the destination action_db will not be available before cloning files to check ACTION_CREATE permission on destination pack.
  • Used delete delete API to remove action from db and disk in case of overwrite.
  • Removed entry point file delete function in case of source and destination action runners are not same.
  • Also, done other minor changes as said.
    Please review. Thanks.
    I couldn't resolve why CI integration check is failing.

@mahesh-orch
Copy link
Contributor Author

@m4dcoder

  • I have refactored the action clone method according to the pseudo code you provided.
  • Used action create API and action delete API.
  • I have refactored existing unit tests as well as added new unit tests as you suggested.
  • Renamed unit test name as test_clone_overwrite_exception_destination_recovered under st2api/tests/unit/controller/v1/test_actions.py, as we are deleting destination action along with files if it exists and overwrite flag is true.
  • Clean up code in st2client/commands/action.py and did deduplicate self.manager.clone.
  • Didn't clean up under st2common/services/packs.py to use dest_action_db to retrieve dest_entry_point and dest_metadata_file as to keep clone_actions function testable.
  • Checking for workflows directory exist, if it isn't then creating it. Added separate unit test to this change.
  • Also, done other minor fixes
  • Wanted to inform that the existing workflows directory has permission mode 775 and if it doesn't exist and we create it then it has permission mode 731 even if I mention it 775 while creating directory.
    Please review the changes. Thanks.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one suggestion. What else is left before we can merge this?

@cognifloyd
Copy link
Member

Hmm. looks like we at least need the /~https://github.com/StackStorm/st2-rbac-backend PR for the tests for this.
Also, @m4dcoder have your concerns been addressed here?

I'm thinking this needs to be bumped to 3.7.0.

…nager.clone`

Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
…ing cloned action database entry in case of exception in operation
Copy link
Contributor

@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mahesh-orch Why do we need to add this file in the PR? st2tests/st2tests/fixtures/packs/dummy_pack_23/actions/workflows/init.py

@mahesh-orch
Copy link
Contributor Author

@m4dcoder

  • I have tested and committed simplified logic under st2common/services/packs.py as per your suggestion.
  • As git isn't accepting empty folder workflows which is needed to clone files at st2tests/st2tests/fixtures/packs/dummy_pack_23/actions/workflows, so, I have created empty file __init__.py again.
    Please review.

@m4dcoder m4dcoder merged commit bcec9f6 into StackStorm:master Dec 1, 2021
@m4dcoder m4dcoder deleted the clone_action_api branch December 1, 2021 19:11
@arm4b
Copy link
Member

arm4b commented Dec 9, 2021

@mahesh-orch @m4dcoder Thanks for the massive work here!
The PR is missing the Changelog. Please add it in another PR so we'll track record of the changes for the st2.

@mahesh-orch
Copy link
Contributor Author

@armab Sure, I will create new PR for adding CHANGELOG. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API CLI feature size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants