fog / fog-google

Fog for Google Cloud Platform
MIT License
98 stars 146 forks source link

Make insert_server support ShieldedInstanceConfig #509

Closed lcy0321 closed 3 years ago

lcy0321 commented 3 years ago

google-api-client support shielded_instance_config since 0.44.2 googleapis/google-api-ruby-client@9678faba19830db0671870acca86794fbc83cb42

It is used to create GCE instances with "Shielded VM" feature https://cloud.google.com/security/shielded-cloud/shielded-vm

lcy0321 commented 3 years ago

I found that google-api-client required ruby 2.4 since 0.33.0 googleapis/google-api-ruby-client@bc573bf987b7ddc4ec912c9d1096e30ccf679d27

So the CI failed in Ruby 2.3 and JRuby 9.1(which support only Ruby 2.3)

Since Ruby 2.3 has been EOL at 2019-03-31 https://www.ruby-lang.org/en/downloads/branches/

Maybe we can drop the support for Ruby 2.3? Then remove Ruby 2.3 and update JRuby to 9.2 for CI environments?

I'll make the change first and see if the CI can pass.

Temikus commented 3 years ago

If Ruby 2.3 is EOL it makes sense to drop it in the newest minor version. Lemme kick off the Integration tests and see how it goes.

lcy0321 commented 3 years ago

Hi @Temikus, thanks your attention. I see the integration test was failed. Though I can't see the test result, I guess it might be because the version of Ruby in Ubuntu 16.04 is 2.3:

root@9823b42bc4e9:/# ruby --version
ruby 2.3.1p112 (2016-04-26) [x86_64-linux-gnu]

Maybe we should find a way to install Ruby 2.4 into the Docker image.

lcy0321 commented 3 years ago

I'll make it install Ruby 2.4 from Brightbox Ruby NG PPA in the Docker.

lcy0321 commented 3 years ago

@Temikus Still failed in Concourse CI 😢. I can't see the test result, need your help to find out what's wrong. Thanks.

Temikus commented 3 years ago

@lcy0321 So sorry, I thought you were a part of the org for some reason.

Here's the log for now and I'll check out what broke in terms of sharing.

Ruby version:
ruby 2.3.1p112 (2016-04-26) [x86_64-linux-gnu]
Bundler version:
Bundler version 2.0.2
Exporting bundler options...
You are replacing the current local value of path, which is currently nil
You are replacing the current local value of bin, which is currently nil
Checking dependencies...
Resolving dependencies...
Bundler can't satisfy your Gemfile's dependencies.
Install missing gems with `bundle install`.
Don't run Bundler as root. Bundler can ask for sudo if it is needed, and installing your bundle as root will break this application for all non-root users on this machine.
Fetching gem metadata from https://rubygems.org/......
Resolving dependencies...
Bundler could not find compatible versions for gem "ruby":
  In Gemfile:
    ruby

    fog-google was resolved to 1.11.0, which depends on
      ruby (~> 2.0)

    fog-google was resolved to 1.11.0, which depends on
      google-api-client (< 0.51, >= 0.44.2) was resolved to 0.50.0, which depends on
        ruby (>= 2.4)

    minitest-reporters was resolved to 1.4.2, which depends on
      minitest (>= 5.0) was resolved to 5.14.2, which depends on
        ruby (< 3.1, >= 2.2)

    webmock was resolved to 3.10.0, which depends on
      ruby (>= 2.0)
lcy0321 commented 3 years ago

Thanks for your help.

I have changed Ruby version from 2.3 to 2.4 in the Dockerfile. But seems that the CI pull the Docker image that is already on DockerHub (rubyfog/fog-google).

Temikus commented 3 years ago

@lcy0321 I'll sort that bit out. Can I ask you to remove the Dockerfile changes from this PR so we can merge cleanly?

I'll aim to carry out upgrades by tomorrow at the latest.

lcy0321 commented 3 years ago

Sure, I've roll-backed the changes. Thanks for your help.

codecov[bot] commented 3 years ago

Codecov Report

Merging #509 (38df8d0) into master (9fe8072) will decrease coverage by 0.30%. The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
- Coverage   88.80%   88.49%   -0.31%     
==========================================
  Files         341      311      -30     
  Lines        5772     5217     -555     
==========================================
- Hits         5126     4617     -509     
+ Misses        646      600      -46     
Impacted Files Coverage Δ
lib/fog/compute/google/requests/insert_server.rb 80.85% <50.00%> (-1.38%) :arrow_down:
lib/fog/google/requests/sql/delete_user.rb 100.00% <100.00%> (ø)
...e/google/requests/set_target_http_proxy_url_map.rb 77.77% <0.00%> (-9.73%) :arrow_down:
.../google/requests/set_target_https_proxy_url_map.rb 77.77% <0.00%> (-9.73%) :arrow_down:
...g/compute/google/requests/set_instance_template.rb 50.00% <0.00%> (-3.85%) :arrow_down:
lib/fog/google/shared.rb 55.67% <0.00%> (-3.03%) :arrow_down:
lib/fog/compute/google/models/operation.rb 80.39% <0.00%> (-1.61%) :arrow_down:
lib/fog/compute/google/models/server.rb 79.14% <0.00%> (-0.38%) :arrow_down:
lib/fog/google/sql/mock.rb 61.11% <0.00%> (ø)
lib/fog/google/pubsub/mock.rb 62.50% <0.00%> (ø)
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6a17261...38df8d0. Read the comment docs.

Temikus commented 3 years ago

Ok, now things are looking much better but the CI fails because the new lib has introduced incompatible changes for our SQL module:

TestSQLUsers
  test_users                                                     ERROR (268.37s)
Minitest::UnexpectedError:         ArgumentError: wrong number of arguments (given 4, expected 2)
            /tmp/build/7e99fb29/bundle/ruby/2.5.0/gems/google-api-client-0.50.0/generated/google/apis/sqladmin_v1beta4/service.rb:1637:in `delete_user'
            /tmp/build/7e99fb29/src/fog-google/lib/fog/google/requests/sql/delete_user.rb:11:in `delete_user'
            /tmp/build/7e99fb29/src/fog-google/lib/fog/google/models/sql/user.rb:27:in `destroy'
            /tmp/build/7e99fb29/src/fog-google/test/integration/sql/test_users.rb:31:in `test_users'

I can tackle it but tomorrow. Otherwise - you're welcome to take a swing if you want. It's probably just changing the parameters.

lcy0321 commented 3 years ago

It looks like some of the parameters of delete_user() have been changed from positional to named. I've tried to fix it.

Temikus commented 3 years ago

@lcy0321 Thank you so much!

Looks like that fixed it. I'm not sure why concourse didn't refer to the new build ref properly. Lemme rerun just in case and I think we're good to merge.

Temikus commented 3 years ago

Alright, SQL is now passing successfully, merging.

Temikus commented 3 years ago

I'll work on a new release now.

lcy0321 commented 3 years ago

Thank you so much. After the release, I'll also create a PR in mitchellh/vagrant-google to make it able to set Shielded VM configurations.