Arvados

We had discussed removing "image" from the config. Looking at the live example in the test (azure_test.go lines 113-121), it is expecting that value in the config, which is used in TestCreate to call AzureInstanceSet.Create. I imagine this code will change, just wondering if I should break the live test for now or if I should change anything in AzureInstanceSet.Create (i.e. the CreatOption parameter in azure.go line 383).

It looks like that test case gets the image ID from the map[string]interface{} and won't be affected by removing the image field from the normal config struct.

• add comment in example azconfig.yml ("VM image to use in test suite")
• rename it to something like ImageIDForTestSuite
• rearrange azconfig.yml so the image ID is outside the driver config like it is in real life:

  ImageID: https://....
  DriverParameters:
    SubscriptionID: ...
    Region: ...
  

A few more questions before review:

• Added a decode hook for arvados.Duration so that mapstructure can decode it appropriately. Right now it is in the duration.go file but not a part of the Duration struct, since it doesn't need a Duration object (it would be a static function in my c++ way of thinking). Not sure if this is the right place for it. As of right now it sits in the arvados package, but I could see it going in the config package or maybe some place else.
• Anything regarding packaging that needs to be updated? I'm assuming this isn't currently getting packaged, but don't know if we have something in the works that should be updated.
• Should I run the live tests? Where do I get the config info?

Eric Biagiotti wrote:

• Added a decode hook for arvados.Duration so that mapstructure can decode it appropriately. Right now it is in the duration.go file but not a part of the Duration struct, since it doesn't need a Duration object (it would be a static function in my c++ way of thinking). Not sure if this is the right place for it. As of right now it sits in the arvados package, but I could see it going in the config package or maybe some place else.

Darn, too bad we don't have the option of using something like the json.Unmarshaler interface.

Yes, I think arvados.WhateverDecodeHookFunc() is the right place. (arvados.StringOrNumberToDurationDecodeHookFunc()?) I think if we test int(1), float64(1.23), json.Number(1.23), and "1.23s", then it should work with Go literals like {"foo":1} as well as anything coming out of the JSON decoder.

• Anything regarding packaging that needs to be updated? I'm assuming this isn't currently getting packaged, but don't know if we have something in the works that should be updated.

No packaging updates needed -- the dispatcher binary gets built from lib/dispatchcloud, and it looks like you've already updated that.

Ran the live tests successfully.

Latest: ff241c64
Tests: https://ci.curoverse.com/view/Developer/job/developer-run-tests/1066/

Also, you mentioned writing a helper function for dispatchcloud tests / writing tests for json decoding. Would you like this to be a part of this issue? Should I make a new story? Depending on your answer, this is ready for review. Thanks!

Eric Biagiotti wrote:

Also, you mentioned writing a helper function for dispatchcloud tests / writing tests for json decoding. Would you like this to be a part of this issue? Should I make a new story? Depending on your answer, this is ready for review. Thanks!

No, I was just thinking of test suites with driver parameters that now need to be JSON-encoded before passing to (cloud.Driver)InstanceSet(). It seems there aren't any, so there's nothing to do.

That said, we might as well preserve the "load params into struct" part of stub_driver.go, instead of ignoring params -- instead of this:

-       return &sis, mapstructure.Decode(params, &sis)
+       return &sis, nil

...maybe this:

-       return &sis, mapstructure.Decode(params, &sis)
+       return &sis, json.Unmarshal(params, &sis)

Am I missing something, or is this code in azure_test.go superfluous? It looks like azcfg never gets used, and if there's an Unmarshal() error, it would be caught by newAzureInstanceSet() on the following line.

                var azcfg azureInstanceSetConfig
                err = json.Unmarshal(exampleCfg.DriverParameters, &azcfg)
                if err != nil {
                        return nil, cloud.ImageID(exampleCfg.ImageIDForTestSuite), cluster, err
                }

Nit: This comment on Set() was obviously copied from Value, but Set is also part of the flag.Value interface, which might still be worth mentioning.

-// Value implements flag.Value
+// Set sets the current duration by parsing the string using time.ParseDuration

Doc comments should be full sentences (some are missing final periods).

Can delete lib/cloud/gocheck_test.go now that there are no tests here.

Tom Clegg wrote:

Eric Biagiotti wrote:

Also, you mentioned writing a helper function for dispatchcloud tests / writing tests for json decoding. Would you like this to be a part of this issue? Should I make a new story? Depending on your answer, this is ready for review. Thanks!

No, I was just thinking of test suites with driver parameters that now need to be JSON-encoded before passing to (cloud.Driver)InstanceSet(). It seems there aren't any, so there's nothing to do.

That said, we might as well preserve the "load params into struct" part of stub_driver.go, instead of ignoring params -- instead of this:

[...]

...maybe this:

[...]

Good call, fixed in: fba0cd4c

Am I missing something, or is this code in azure_test.go superfluous? It looks like azcfg never gets used, and if there's an Unmarshal() error, it would be caught by newAzureInstanceSet() on the following line.

[...]

Nit: This comment on Set() was obviously copied from Value, but Set is also part of the flag.Value interface, which might still be worth mentioning.
[...]

Doc comments should be full sentences (some are missing final periods).

Can delete lib/cloud/gocheck_test.go now that there are no tests here.

Haha, that was all debug code that I ended up forgetting about then fixing up as if it was needed :( This and remaining comments fixed in: 8be52d22

I also improved another comment in b7f7154a

Tests: https://ci.curoverse.com/view/Developer/job/developer-run-tests/1069/

LGTM @ b7f7154a7, thanks

Fixed dispatchcloud/worker/PoolSuite tests in c9127a21 and 1bd522fd. StubDriver.InstanceSet now allows for nil params. Seems like a better solution than forcing tests to supply a json.RawMessage that don't need to.

Agreed. LGTM, thanks