WIP (DIO-911) Include job_id in ABS --json output

[nwolfe]

Prior to this commit, when using --service=abs and --json the job_id
value is not included in the JSON blob, which makes it difficult for
calling code to grab both the job_id and the hosts without text
processing the log messages printed to standard error.

---

Based on #91 but not strictly related to those changes.

---

⚠️ I could use some design direction here, and determine how we want to update tests ⚠️

---

Prior to this change the output was:

$ floaty get centos-7-x86_64 --json
--
Requesting VMs with job_id: 1597884146476.  Will retry for up to an hour.
Waiting 1 seconds to check if ABS request has been filled.  Queue Position: 277... (x1)
Waiting 2 seconds to check if ABS request has been filled.  Queue Position: 277... (x2)
Waiting 3 seconds to check if ABS request has been filled.  Queue Position: 277... (x3)
Waiting 4 seconds to check if ABS request has been filled.  Queue Position: 277... (x4)
{
  "centos-7-x86_64": [
    "near-betterment.delivery.puppetlabs.net"
  ]
}

But that does not play well with calling code as the job_id value is very important, yet buried within the log message sent to stderr.

A calling script would have to capture stderr and text-match to extract the job_id.

The job_id is important because callers want to be good citizens and subsequently call floaty delete <job_id> to return everything back to the pool.

With these changes we now get the job_id included in the JSON blob:

$ floaty get --service=abs --user=nwolfe --token=**** centos-7-x86_64 --url=https://cinext-abs.delivery.puppetlabs.net/api/v2 --json 2>/dev/null
{
  "job_id": "1598302813223",
  "centos-7-x86_64": [
    "cosmic-handgun.delivery.puppetlabs.net"
  ]
}

---

⚠️ The failing spec test is trying to ensure that ABS' hash matches VMPooler's hash:

Failures:

  1) ABS#format returns an hash formatted like a vmpooler return
     Failure/Error: expect(vmpooler_formatted_response).to eq(vmpooler_formatted_compare)

       expected: {"centos-7.2-x86_64"=>{"hostname"=>["aaaaaaaaaaaaaaa.delivery.puppetlabs.net", "aaaaaaaaaaaaaab.deliv....net"]}, "ok"=>true, "ubuntu-7.2-x86_64"=>{"hostname"=>["aaaaaaaaaaaaaac.delivery.puppetlabs.net"]}}
            got: {"centos-7.2-x86_64"=>{"hostname"=>["aaaaaaaaaaaaaaa.delivery.puppetlabs.net", "aaaaaaaaaaaaaab.deliv...567890", "ok"=>true, "ubuntu-7.2-x86_64"=>{"hostname"=>["aaaaaaaaaaaaaac.delivery.puppetlabs.net"]}}

       (compared using ==)

       Diff:
       @@ -1,2 +1,3 @@
        "centos-7.2-x86_64" => {"hostname"=>["aaaaaaaaaaaaaaa.delivery.puppetlabs.net", "aaaaaaaaaaaaaab.delivery.puppetlabs.net"]},
       +"job_id" => "1234567890",
        "ok" => true,

     # ./spec/vmfloaty/abs_spec.rb:32:in `block (3 levels) in <top (required)>'

Finished in 0.62604 seconds (files took 0.63166 seconds to load)
113 examples, 1 failure

but I'm not sure that's a correct invariant really 🤔

[nwolfe]

Ping @briancain @puppetlabs/dio for review/help

[briancain]

The change makes sense to me from a design perspective.

That test is a little strange. Given that the ABS and main vmpooler service are technically two different services, it would probably be better to just check that the resulting hash includes the same information and expected keys, but could be updated to be looser so that it isn't a hard restriction that the two return the exact same result for when new keys need to be surfaced in the ABS that don't make sense for the pooler service.

[nwolfe]

Seems like at one point the desire was to have the JSON blobs be the same across all services (vmpooler, nspooler, ABS) perhaps?

But some services have additional information that is not relevant for the others, which is the point we're at now with this PR.

Not sure how we want to address this.

We could give up on this and just say users have to condition their output based on the service argument, or we could come up with a new JSON "shape" that is service-agnostic and supports everything, or something else... 🤔

[nwolfe]

Code change suggestions welcome as I'm new to this codebase 😄

[briancain]

Hm yeah, I've unfortunately lost some context here so I might not be the best authority for this change 🙈 It seems like this translate function just remaps keys to look the same as the other services, so that the code ends up being the same here. I think that's probably a good thing to preserve, otherwise it might look really messy handling all the different kinds of services vmfloaty now interacts with.

That being said, I don't see a problem with adding things to the data structure, as long as the expected keys exist, I don't know if anything would break? I don't really have the time to check on that, but if others with more context would like to weigh in that would be great!

A more drastic change would be defining a Model class that explicitly defines the expected keys returned from an API. That would be a much better interface than parsing JSON and storing it as a Hash and 🙏 ing that it conforms to the right structure. You can see an example of this in the vagrant codebase. We recently had this issue, and I think this is a clean way to solve it. Then if there are any service specific values, it can be addressed this way without doing any awful key remapping.

[nwolfe]

Ping @austb and @barriserloth as I think you have code that is parsing the JSON output of vmfloaty.

Would be good to get your inputs on this so we can potentially account for all our use-cases and hopefully not break existing usages.

[nwolfe]

I don't see a problem with adding things to the data structure, as long as the expected keys exist, I don't know if anything would break?

Yeah I agree.

The way this could break existing usages is if it was assumed that the ONLY thing in the JSON blob is platform -> hosts pairings.

Now there will be this new top-level job_id: 1234 value, which might break those loops if they're not conditioning on value.is_a? Array or something similar.

Not sure if this is a concern or not though.

If it is, we could do something like put the current hosts-only blob behind a hosts key or something in the top-level JSON 🤷‍♂️ That way it's future-proof for adding new top-level keys. There are other ways we can address all this too, just spitballing.

[briancain]

I think maybe the safest bet forward might be going with the Module class. Then you can define the structure internally, so that you are at least assured when you grab data from the poolers, it can match the Model defined. That model could also do the work to make sure all services end up structured the same in vmfloaty so that the code that uses that structure doesn't have to change, or be limited by issues such as this with using the direct JSON result as a Hash. But I also totally understand that's a bit more work! 😅

[nwolfe]

I'm not sure I'm going to have the time to understand, implement, and test this stuff, so I will likely just depend on my fork for the time being until we can support this stuff for realz.

Happy to work with DIO/others to make these improvements when we get there.

[briancain]

@nwolfe - no worries, that's understandable. I think this could probably be fixed by keeping vmfloaty how it is (with just passing along the response Hash), it might just be a bit trickier for the reasons you've already mentioned! 😄

[nwolfe]

See where/how this is used: https://github.com/puppetlabs/PipelinesInfra/pull/1680

[genebean]

I have no problems adding things to the output. That said, since it is a potentially breaking change we could do a major version bump at release to indicate as much and that way people should be safe from a standard Gemfile entry pulling it in unexpectedly. Does that seem reasonable to you @briancain ?

[austb]

From my view, it's still a 0. version so every release can be a breaking release, so I'd be fine with just a 0.12.0 but that sort of easy-out of semver requirements is the reason there are so many version 0 ruby projects. But on the specifics, it doesn't look like this would break my usage of the json output.

[briancain]

One thing I'd be careful of is that I know a handful of people treat the CLI output as if its an API. Just recently there were a few PRs that did this, and I know a few others had done that too. It might be worth looking into how to add this without changing the output too much! Or at least letting people know internally that a change is coming 😄

[sbeaulie]

similar to @nwolfe's suggestion, I would suggest a variable after the --json similar to k8's -o flag (output option) where you can specify wide|json|yaml

something like

--json vmpooler (current behavior)
--json abs (adds job_id and other elements expected by beaker-abs)
--json foo (or bar, baz)