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

joyent/node-triton#128 triton ls state=provisioning` doesn't filter on state #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joyent-automation
Copy link
Contributor

TritonDataCenter/node-triton#128 triton ls state=provisioning` doesn't filter on state

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/1606/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@marsell commented at 2017-03-03T08:50:05

Patch Set 1: Code-Review+1 Integration-Approval+1

Normally I'd ask for a test for this, but it probably would be tricky to do reliably.

@YangYong3 commented at 2017-03-15T00:16:21

Uploaded patch set 2: Patch Set 1 was rebased.

@marsell commented at 2017-04-04T23:41:19

Patch Set 2:

Yang Yong, are you planning to integrate this? It's been a while, so I'm wondering if this is in limbo.

@trentm commented at 2017-04-05T00:05:13

Patch Set 2:

Marsell,

Have you tested this? I wasn't actually sure that querying for state=provisioning to VMAPI would have results.

@marsell commented at 2017-04-05T11:06:40

Patch Set 2: -Integration-Approval

Have you tested this? I wasn't actually sure that querying for
state=provisioning to VMAPI would have results.

Turns out you're right. It looked like it should work with vmapi, and that the docs were wrong, but nope: VALID_VM_STATES in common/validation.js doesn't allow it.

Is there a good reason we shouldn't add it to vmapi -- other than dealing with the ripple to other callers? Right now cloudapi gives an odd result when attempting to filter by 'provisioning': it returns all running VMs, not just VMs provisioning.

I think it'd be useful to allow filtering by provisioning, but the alternative is we disallow filtering by 'provisioning' in cloudapi; at least the results will be consistent with what was requested.

Unrelatedly, I was quite wrong about this being tricky to add tests for: it's not. If we decide to add support to vmapi for filtering by 'provisioning', I'm okay with driving all of this the rest of the way if need be (tests, dealing with other vmapi callers, etc).

@trentm trentm removed their request for review October 21, 2019 18:40
@bahamat bahamat self-requested a review May 23, 2021 06:01
@bahamat bahamat self-assigned this May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants