[google] Fix Google Engine tests #2509

allomov · 2013-12-26T15:22:46Z

Google Engine live tests fail after v1 update. More fixes coming.

… names to their names

coveralls · 2013-12-26T15:31:44Z

Coverage remained the same when pulling e809e0e on allomov:google-tests into 7880a55 on fog:master.

allomov · 2013-12-26T15:38:48Z

@icco I've found interesting bug/feature in GCE REST API. Different types of requests are using different object representations in fields with the same names: for instance Instances#get will return URL-like zone, but in the same time Instances#reset call requires you to specify zone as a simple name. So if you call

compute.services.each { |s| s.reset }

, you will get this error:

Parameter 'zone' has an invalid value: https://www.googleapis.com
/compute/v1/projects/<project-name>/zones/us-central1-a. 
Must match: /^[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?$/. (ArgumentError)

allomov · 2013-12-26T15:40:13Z

So I've put this fast solution here: https://github.com/allomov/fog/blob/e809e0ef567c1e514b3a360553aacce0242e59c6/lib/fog/google/helpers/attribute_convertor.rb

coveralls · 2013-12-26T15:50:17Z

Coverage remained the same when pulling ce570cf on allomov:google-tests into 7880a55 on fog:master.

coveralls · 2013-12-26T16:08:28Z

Coverage remained the same when pulling aef8974 on allomov:google-tests into 7880a55 on fog:master.

coveralls · 2013-12-26T17:23:17Z

Coverage decreased (-12.45%) when pulling ffce747 on allomov:google-tests into 7880a55 on fog:master.

icco · 2013-12-26T17:32:58Z

lib/fog/google/models/compute/disk.rb

-          end
-          operation
+          operation = service.operations.new(service.delete_disk(name, zone_name).body)
+          operation.wait_for { ready? }


This is slightly different then what currently exists. To be the same it should be operation.wait_for { !pending? }.

icco · 2013-12-26T17:43:03Z

As for the zones bug, we've come across that before. I had gone through any made it so all requests at the time took just a zone name from our perspective and tweaked it to fit the API. It looks like some of that got deleted (doh). The key when reading the docs is if it wants a zone resource, you give it the full URL, and if it just wants the zone name, then you do that. The best place to do this is in the request.

In a perfect world, a user would pass in either a url or a shortened zone name into the model, and the request would clean it up and make it perfect.

allomov · 2013-12-27T12:37:48Z

@icco I don't like the idea to have undetermined value format inside zone_name or machine_type fields. You can divided such type of fields into zone_url and zone_name (for instance) and set them after zone attribute is set appropriately, but you shouldn't have in zone_name either URL or name and delegate request to deal with it.

allomov · 2013-12-27T12:42:19Z

I see this solutions:

fast one: use converter to get stored attribute in one way.
other one: add zone_url and zone_name getters (maybe also hide zone getter to prevent misunderstandings) which will work using @zone variable that will be set with zone=

coveralls · 2013-12-27T12:55:44Z

Coverage remained the same when pulling 7e5dece on allomov:google-tests into 7940b30 on fog:master.

icco · 2013-12-27T18:46:05Z

lib/fog/google/compute.rb

      request :set_metadata

+


nit: delete extra line.

icco · 2013-12-27T18:53:04Z

@allomov, I don't think I understand what you're trying to do with the converter or what you're saying about zones. The model is the user interface, we take in data from the user, validate it and pass it on to the request, which builds the request and sends it off to GCE. We shouldn't care what the user passes into our model, as long as we are consistent with what we return to them.

Maybe write a test explaining the bug and the desired outcome?

allomov · 2013-12-27T20:17:14Z

@icco I think this illustrates what I am trying to do: https://github.com/Altoros/bosh/blob/google-cpi/bosh_google_cpi/spec/integration/remove_all_instances_spec.rb#L54-L56
without converter you'll have an error:

Parameter 'zone' has an invalid value: https://www.googleapis.com
/compute/v1/projects/<project-name>/zones/us-central1-a. 
Must match: /^[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?$/. (ArgumentError)

If you use instance that is creates by get request, then you'll have zone field with URL mapped to zone_name. Obviously you can solve this part in delete request.

But the problem is that zone URL is stored in zone_name field and zone name is not zone URL.

allomov · 2013-12-27T21:24:44Z

@icco this issue is solved in different parts in different way. for instance in disk. I think such thing should be done for all objects and fields that need it. what way do you prefer ?

allomov · 2013-12-27T22:18:40Z

@icco I strongly recommend to distinguish zone_name and zone_url attributes fields to prevent misunderstandings in future. I think I will create PR to show you what I mean.

icco · 2013-12-28T00:01:35Z

so, my point is that we shouldn't need to show the zone url to users. The zone name is just a shortened representation of the zone url. I personally like how we do it in Disk, and would love if that was consistent across the project. At one point it was, but over time I guess we've gotten sloppy. I guess my problem with the converter is it doesn't make much sense to me and seems far too generic. When else would you use something like that? Why not just put it into the zone method like we do with disk? I guess my point is I'd prefer to be explicit about the transformation that we're doing to the data we're getting back.

allomov · 2014-01-08T11:47:27Z

@icco ok, I guess we can go with it :)
Will change my approach

coveralls · 2014-01-08T11:54:02Z

Coverage decreased (-12.56%) when pulling 9ff70d4 on allomov:google-tests into 2560ac1 on fog:master.

allomov · 2014-01-08T13:00:04Z

@icco I want to follow least astonishment principle in the question with urls and names. Personally I was surprised with fact that zone can take either zone url either zone name. I removed attributes converter and came with following solution: https://github.com/allomov/fog/blob/google-tests/lib/fog/google/requests/compute/reset_server.rb#L12-L13 also I've added requests helper to store common helper methods.

allomov · 2014-01-08T13:03:12Z

@icco if you agree with this approach, I'll move other requests to use this convention.

coveralls · 2014-01-08T13:04:39Z

Coverage decreased (-0.45%) when pulling 134967e on allomov:google-tests into 2560ac1 on fog:master.

icco · 2014-01-08T17:03:45Z

Sounds good to me. Thanks!

coveralls · 2014-01-08T19:27:42Z

Coverage increased (+0.04%) when pulling c48392d on allomov:google-tests into 7f7665a on fog:master.

coveralls · 2014-01-11T12:47:30Z

Coverage decreased (-12.7%) when pulling e8ca5b9 on allomov:google-tests into 612e33a on fog:master.

… after they exited with exceptions

coveralls · 2014-01-14T19:05:06Z

Coverage decreased (-0.01%) when pulling 28e3df3 on allomov:google-tests into 8e393db on fog:master.

allomov · 2014-01-15T11:33:07Z

I close this PR as far I as merged this branch to #2501.

geemus · 2014-01-15T15:57:31Z

Thanks!

On Wed, Jan 15, 2014 at 5:33 AM, Alexander Lomov
notifications@github.comwrote:

Closed #2509 #2509.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2509
.

allomov added 2 commits December 26, 2013 14:20

fix block sequence for server tests for google

471599d

add reset method to server, add convertor to modify url like resource…

e809e0e

… names to their names

fix misspellint

ce570cf

fix one more misspelling

aef8974

try to fix error with disks and storage

ffce747

icco reviewed Dec 26, 2013
View reviewed changes

fix uncorrect way of wait_for in google compute disk

8569568

Merge branch 'master' into google-tests

7e5dece

icco reviewed Dec 27, 2013
View reviewed changes

lib/fog/google/compute.rb

request :set_metadata

Copy link

Member

icco Dec 27, 2013

Choose a reason for hiding this comment

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

nit: delete extra line.

ghost assigned icco Dec 27, 2013

Merge branch 'master' into google-tests

9ff70d4

remove attribute converter and add requests helper

4574ff4

fix tests after merging with master

134967e

Merge branch 'master' into google-tests

c48392d

Merge branch 'master' into google-tests

ffafb71

This was referenced Jan 9, 2014

[google|storage] Add put_object_acl request #2556

Merged

How to use ssh_add_key for Google server? #2566

Closed

allomov added 3 commits January 11, 2014 11:03

Merge branch 'master' into google-tests

7dc9b74

refactor requests to use helper converter to work with urls and names

3279551

fix google app engine tests

e8ca5b9

allomov added 3 commits January 14, 2014 14:04

fix disk names to have no interseptions with disk names that was left…

7d6e56f

… after they exited with exceptions

fix 3 insert image test

56a1421

merge with fog master branch

28e3df3

allomov mentioned this pull request Jan 15, 2014

[google|compute] v1 update and enhancement (fixing tests, Networks setup, Server metadata update, Disk attachment, etc.) #2501

Closed

allomov closed this Jan 15, 2014

allomov referenced this pull request Feb 6, 2014

[google] Create zone and zones models

fae6583

allomov deleted the google-tests branch June 6, 2014 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google] Fix Google Engine tests #2509

[google] Fix Google Engine tests #2509

allomov commented Dec 26, 2013

coveralls commented Dec 26, 2013

allomov commented Dec 26, 2013

allomov commented Dec 26, 2013

coveralls commented Dec 26, 2013

coveralls commented Dec 26, 2013

coveralls commented Dec 26, 2013

icco Dec 26, 2013

icco commented Dec 26, 2013

allomov commented Dec 27, 2013

allomov commented Dec 27, 2013

coveralls commented Dec 27, 2013

icco Dec 27, 2013

icco commented Dec 27, 2013

allomov commented Dec 27, 2013

allomov commented Dec 27, 2013

allomov commented Dec 27, 2013

icco commented Dec 28, 2013

allomov commented Jan 8, 2014

coveralls commented Jan 8, 2014

allomov commented Jan 8, 2014

allomov commented Jan 8, 2014

coveralls commented Jan 8, 2014

icco commented Jan 8, 2014

coveralls commented Jan 8, 2014

coveralls commented Jan 11, 2014

coveralls commented Jan 14, 2014

allomov commented Jan 15, 2014

geemus commented Jan 15, 2014

[google] Fix Google Engine tests #2509

[google] Fix Google Engine tests #2509

Conversation

allomov commented Dec 26, 2013

coveralls commented Dec 26, 2013

allomov commented Dec 26, 2013

allomov commented Dec 26, 2013

coveralls commented Dec 26, 2013

coveralls commented Dec 26, 2013

coveralls commented Dec 26, 2013

icco Dec 26, 2013

Choose a reason for hiding this comment

icco commented Dec 26, 2013

allomov commented Dec 27, 2013

allomov commented Dec 27, 2013

coveralls commented Dec 27, 2013

icco Dec 27, 2013

Choose a reason for hiding this comment

icco commented Dec 27, 2013

allomov commented Dec 27, 2013

allomov commented Dec 27, 2013

allomov commented Dec 27, 2013

icco commented Dec 28, 2013

allomov commented Jan 8, 2014

coveralls commented Jan 8, 2014

allomov commented Jan 8, 2014

allomov commented Jan 8, 2014

coveralls commented Jan 8, 2014

icco commented Jan 8, 2014

coveralls commented Jan 8, 2014

coveralls commented Jan 11, 2014

coveralls commented Jan 14, 2014

allomov commented Jan 15, 2014

geemus commented Jan 15, 2014