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

cmd: Add deployment plan cancel #72

Merged
merged 2 commits into from Dec 5, 2019

Conversation

marclop
Copy link
Collaborator

@marclop marclop commented Dec 4, 2019

Description

Adds the deployment plan cancel command which allows users to cancel
pending plans for specific resources.
The syntax is as follows:
ecctl deployment plan cancel <deployment id> --type <type> --ref-id <id>

Motivation and Context

Implement more of the Deployment resource APIs.

How Has This Been Tested?

$ ecctl deployment plan cancel fc2fe19a8c4f43d385004febe4e63900 --type kibana --ref-id kibana --force
{
  "errors": [
    {
      "code": "deployments.resource_does_not_have_a_pending_plan",
      "fields": null,
      "message": "The Resource does not have a pending plan: DeploymentId [fc2fe19a8c4f43d385004febe4e63900], Kind [kibana], ResourceId: [8d37cff942fb482ba7a3cac10eea943a]"
    }
  ]
}

Types of Changes

  • New feature (non-breaking change which adds functionality)

@marclop marclop added enhancement New feature or request area:tooling labels Dec 4, 2019
@marclop marclop added this to the 33: Dec 04 - Dec 24 milestone Dec 4, 2019
@marclop marclop requested a review from a team as a code owner December 4, 2019 16:02
@marclop marclop self-assigned this Dec 4, 2019
Adds the deployment plan cancel command which allows users to cancel
pending plans for specific resources.
The syntax is as follows:
ecctl deployment plan cancel <deployment id> --type <type> --ref-id <id>

Signed-off-by: Marc Lopez <marc5.12@outlook.com>
@marclop marclop force-pushed the f/add-deployment-plan-cancel branch from 4f47cc7 to 8a9c216 Compare December 4, 2019 16:04
infraredgirl
infraredgirl previously approved these changes Dec 4, 2019
Copy link
Contributor

@infraredgirl infraredgirl left a comment

Choose a reason for hiding this comment

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

Very nice 👍

cmd/deployment/plan/command.go Outdated Show resolved Hide resolved
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
merr = multierror.Append(merr, deputil.NewInvalidDeploymentIDError(params.DeploymentID))
}

if params.Type == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it makes any sense to add some checks here about the accepted types. I know we are using this in a few places in the code so we could create a common check. Ignore me if it doesn't worth the time

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We discussed it out of band and agreed that we should't overload this client logic and leave the API to validate the type sent as a queryparam

}

// Validate ensures the parameters are usable by the consuming function.
func (params CancelPlanParams) Validate() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are missing test for this method. I think we should explicitly test this function as we already do in other places rather than indirectly test it

@marclop marclop merged commit 88c7938 into elastic:master Dec 5, 2019
@marclop marclop deleted the f/add-deployment-plan-cancel branch December 5, 2019 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants