End lease using lease ID

joshmarsh commented 4 years ago

Proposed changes

'dce leases end' now accepts a leaseID arg instead of principalID & accountID flags

Types of changes

[ ] Bugfix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Refactor (changes to code, which do not change application behavior)

Checklist

[x] I have filled out this PR template
[x] I have read the CONTRIBUTING doc
[ ] I have added automated tests that prove my fix is effective or that my feature works
[x] I have added necessary documentation (README.md, inline comments, etc.)
[x] I have updated the CHANGELOG.md under a ## next release, with a short summary of my changes

Relevant Links

https://github.com/Optum/dce-cli/issues/63

Further comments

I considered keeping the --principalID and --acountID flags for backward compatibility, but I don't really think they make sense. I don't think a user would ever want to delete a lease by those values, and an admin could easily look up the lease ID. Let me know if you have a different opinion.

eschwartz commented 4 years ago

@joshmarsh if you have time, it would be really great to get some test coverage here. I've been leaning towards integration-style tests, that cover everything up to network requests.

Check this out: https://github.com/Optum/dce-cli/blob/e4a2a6990dd592e2c9a5ca1718f04e64d6ca5182/tests/integration/leases_test.go#L17

        leasesCreateTest(t, &leaseCreateTestCase{
            commandArgs: []string{
                "leases", "create",
                "-p", "test-user",
                "-b", "100", "-c", "USD",
                "-e", "test@example.com",
                "--expires-on", "1h",
            },
            expectedCreateLeaseRequest: operations.PostLeasesBody{
                BudgetAmount:             aws.Float64(100),
                BudgetCurrency:           aws.String("USD"),
                BudgetNotificationEmails: []string{"test@example.com"},
                ExpiresOn:                float64(time.Now().Add(time.Hour).Unix()),
                PrincipalID:              aws.String("test-user"),
            },
            mockAccountID: "123456789012",
            expectedJSONOutput: map[string]interface{}{
                "accountId":                "123456789012",
                "budgetAmount":             float64(100),
                "budgetCurrency":           "USD",
                "budgetNotificationEmails": []interface{}{"test@example.com"},
                "principalId":              "test-user",
                "expiresOn":                time.Now().Add(time.Hour).Unix(),
            },
        })

that gives us a real nice "if I run X CLI command, it should send Y API request, and output Z to stdout"

joshmarsh commented 4 years ago

Hey @joshmarsh -- good to code with you again!

Do we think we could do this without introducing a breaking change? ie accept both formats:
dce leases end <lease-id>
and
dce leases end -p <principalId> -a <accountId>
If having an optional argument for this command is not possible, second best option would be to accept a new flag for the --id parameter

Are you sure we want to keep those flags? We originally added them to be consistent with the api, but I'm not sure if there's a good use case for them.

eschwartz commented 4 years ago

Are you sure we want to keep those flags?

Yeah, I think so, it seems like a valid use case ("please end the lease for this user using this account"). Ideally, we'd support other query params for that API too, like "please end the active lease for Edan", "please end the lease for account=123", or "please end my lease".

And generally, just trying to avoid breaking change (we probably have existing systems relying on this behavior)

eschwartz commented 4 years ago

Thanks for making that change, @joshmarsh . Do you think we could get some test coverage around this? See my comment here: https://github.com/Optum/dce-cli/pull/83#issuecomment-616826821

Optum / dce-cli