Skip to content
This repository has been archived by the owner on May 10, 2021. It is now read-only.

Better output formatting + ability to stop/remove boxes #2

Merged
merged 14 commits into from
May 30, 2016

Conversation

mbr
Copy link
Contributor

@mbr mbr commented May 18, 2016

New output format:

apitest1(295748)
  activeProfile: None
  created: 2016-05-18 21:49:01
  host: vmhost-2-2-1-9
  id: 295748
  ips: 
    private: 10.1.31.4
    public: 134.119.17.112
  isBeingCopied: False
  manualBackupRunning: False
  metadata: 
  name: apitest1
  plan: CloudLevel 1(44)
    cpus: 1
    diskSizeInMB: 102400
    id: 44
    name: CloudLevel 1
    pricePerHour: 0.02
    pricePerHourFrozen: 0.005
    ramInMB: 2048
  recoverymodeActive: False
  running: False
  runningSince: None
  status: CREATING

Additionally, it is possible to remove running boxes thanks to the added stop() API method:

$ jiffybox box delete -f 295748
Really delete box 295748 (apitest1)? [y/N]: y
Waiting for box to become ready
Stopping box 295748 (apitest1)
Removed box 295748 (apitest1)

@t-8ch
Copy link
Contributor

t-8ch commented May 19, 2016

Thank you for the PR! I have some inline remarks.
Cc @maxvozeler

@click.argument('id')
@click.option('--no-confirm', is_flag=True, default=False)
def delete_box(id, no_confirm):
if no_confirm:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to keep the logic of the old delete_box. It is designed to avoid an unnecessary API rountrip and be a direct mapping of the library function.
We could introduce wait-stop-and-delete which uses the new logic. (Better names are welcome)

@mbr
Copy link
Contributor Author

mbr commented May 21, 2016

Hm, I'm guessing you have a vastly higher number of jiffyboxes than I have =). Some comments:

Saving API round-trips is good, I just did not think it was a large issue at that point. I may be wrong, if necessary, I can put it back to one at the cost of a little legibility. Getting the Box info everytime makes for a bit better error messages though.

Regarding list: It does not take terribly long to get the box list (at least with the amount of boxes I'm dealing with) and a confirmation for a command that does not change anything is rare. It's fairly annoying having to type confirm all the time, and bad for scripting.

If you do thing that it is a concern that a tool is taking a while to complete an action (which is not unhead of for unix command line tool), here's what I'd suggest to alleviate the situation (any of those below should suffice):

  • Simply output a status message like "Retrieving box infos, this may take a while...", best written to stderr instead of stdout.
  • Make the api return a generator that outputs each box as it is received, partial output indicating progress.
  • Adding a progress bar (click comes with nice helper functions for those and they'd work especially well with the generator above).

@t-8ch
Copy link
Contributor

t-8ch commented May 30, 2016

@mbr
I would like to keep the confirmation on the list. The documentation explicitly tells to use it sparingly.

Da dieser 
Aufruf sehr zeitintensiv ist, raten wir von der Verwendung zur kontinuierlichen Abfrage des 
Status explizit ab.

How about a jiffybox list --no-confirm option?
We can't use a generator because the API is not streaming. It does its work and gives us the whole response in one swoop.

About the point about roundtrips and better error messages I now agree with you.

@mbr
Copy link
Contributor Author

mbr commented May 30, 2016

@mbr
I would like to keep the confirmation on the list. The documentation explicitly tells to use it sparingly.

It is ultimately your call =)

Da dieser
Aufruf sehr zeitintensiv ist, raten wir von der Verwendung zur kontinuierlichen Abfrage des
Status explizit ab.
How about a jiffybox list --no-confirm option?

I am not sure if that would make a difference to jiffybox, after all someone that hasn't read the API docs (which is probably every user using the tool, since he or she would use it to avoid having to read the docs and implement things him/herself) will not deduce that the status call should be used sparingly solely from the fact that there is a confirmation prompt slapped onto it. So a better solution might be to not introduce a confirmation prompt here, but a particular warning that jiffybox asks for this functionality not to be used excessively?

I do feel that this might be even better solved on jiffybox's end; if they need a rate limit, they could just introduce one - after all, each call to this API needs to be authenticated anyway. "Advising" to "not use the API continuously" is as vague as this criticism of the fact...

We can't use a generator because the API is not streaming. It does its work and gives us the whole response in one swoop.

Showing output fast enough seems to be a minor issue if the required self-limitation is the bigger issues =)

@t-8ch
Copy link
Contributor

t-8ch commented May 30, 2016

Okay, I am conviced :-)
So we remove the whole confirmation business and add a warning to the docs.
Do you want to do it?

@mbr
Copy link
Contributor Author

mbr commented May 30, 2016

See the most recent commits.

However, I've just looked into the docs and it seems there's a sentence missing from your citation above:

Mit diesem Aufruf erhalten Sie eine Übersicht aller eingerichteten JiffyBoxen. Da dieser
Aufruf sehr zeitintensiv ist, raten wir von der Verwendung zur kontinuierlichen Abfrage des
Status explizit ab. Hierfür sollten Sie den spezifischeren Aufruf auf die einzelne JiffyBox
nutzen.

With that in mind, I would interpret this section solely as "if you're polling this for status, this might take longer than you'd like, poll individual boxes for better performance if you do not need all of them". It's either that or their discovery is slow.

In both cases, I'd say that the warning isn't necessarily needed. I'll leave including it up to you (personally I'd just ignore it), you may want to get in touch with jiffybox and ask them about their strange docs (or why they're not providing a python lib in the first place ;))

@t-8ch t-8ch merged commit 3fc2d37 into AmadeusITGroup:master May 30, 2016
@t-8ch
Copy link
Contributor

t-8ch commented May 30, 2016

@mbr
Thanks for your contribution!
(I did some cleanup, but mostly cosmetic)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants