Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement API method resize_disk for managed disks #660

Merged
merged 2 commits into from
Feb 23, 2022
Merged

Implement API method resize_disk for managed disks #660

merged 2 commits into from
Feb 23, 2022

Conversation

vicwicker
Copy link
Contributor

Checklist:

Please check each of the boxes below for which you have completed the corresponding task:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • All unit tests pass locally (after my changes)
  • Rubocop reports zero offenses (after my changes)

Unit Test output:

Finished in 12.4 seconds (files took 1.2 seconds to load)
1020 examples, 0 failures

Coverage report generated for RSpec to /root/workspace/bosh-azure-cpi-release/src/bosh_azure_cpi/coverage. 4277 / 4296 LOC (99.56%) covered.
~/workspace/bosh-azure-cpi-release/src/bosh_azure_cpi

Rubocop output:

Offenses:

lib/cloud/azure/restapi/azure_client.rb:25:3: C: Metrics/ClassLength: Class has too many lines. [1664/1645]
  class AzureClient ...
  ^^^^^^^^^^^^^^^^^
spec/unit/azure_client/get_operation_spec.rb:8:1: C: Metrics/BlockLength: Block has too many lines. [2219/2197]
describe Bosh::AzureCloud::AzureClient do ...
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

187 files inspected, 2 offenses detected

Changelog

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 14, 2022

CLA Signed

The committers are authorized under a signed CLA.

@vicwicker vicwicker changed the title Resize managed disks instead of creating a new one and copying data Add a new API method resize_disk Feb 14, 2022
@vicwicker vicwicker changed the title Add a new API method resize_disk Add a new API method resize_disk for managed disks Feb 14, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 14, 2022

CLA Signed

The committers are authorized under a signed CLA.

@vicwicker vicwicker changed the title Add a new API method resize_disk for managed disks Implement API method resize_disk for managed disks Feb 14, 2022
@beyhan
Copy link
Member

beyhan commented Feb 14, 2022

@vicwicker can you please sign the CLA?

@rkoster rkoster requested review from a team, jpalermo and lnguyen and removed request for a team February 15, 2022 13:40
Copy link
Member

@lnguyen lnguyen left a comment

Choose a reason for hiding this comment

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

Looks good overall, it might be hard to test but can you write some integration tests? If anything if it fails in pipelines we(BOSH team) will fix it

@vicwicker
Copy link
Contributor Author

Looks good overall, it might be hard to test but can you write some integration tests? If anything if it fails in pipelines we(BOSH team) will fix it

@lnguyen I added a integration test: 5a70b1a

Other behaviours raise a Bosh::Clouds::NotSupported exception and I think that's covered by the unit tests and would become redundant with an integration test.

The summary of the new integration test is:

# bundle exec rspec spec/integration/managed_disks_spec.rb --format documentation
...
I, [2022-02-17T08:41:27.191922 #365 #5520] INFO --: Create new managed disk with 10000 MiB
...
I, [2022-02-17T08:41:33.215034 #365 #5520] INFO --: Resize managed disk 'caching:None;disk_name:bosh-disk-data-eb3f7caf-088e-4d0e-b8ad-922d458cf26d;resource_group_name:<redacted>' to 20000 MiB
... 
I, [2022-02-17T08:41:39.270051 #365 #5520] INFO --: Delete managed disk 'caching:None;disk_name:bosh-disk-data-eb3f7caf-088e-4d0e-b8ad-922d458cf26d;resource_group_name:<redacted>'
...
Finished in 20.23 seconds (files took 0.45375 seconds to load)
1 example, 0 failures

@vicwicker vicwicker requested a review from lnguyen February 17, 2022 09:15
@beyhan
Copy link
Member

beyhan commented Feb 17, 2022

In cloudfoundry/bosh-aws-cpi-release#123 we had some findings in the AWS CPI resize implementation which could be useful here. Just adding it as context.

Copy link
Member

@lnguyen lnguyen left a comment

Choose a reason for hiding this comment

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

LGTM

@vicwicker
Copy link
Contributor Author

Is there anything left to do here or could we maybe merge this? Thanks!

@beyhan
Copy link
Member

beyhan commented Feb 22, 2022

There is still one review open. We have a police to have two reviews per pr.

Copy link
Member

@jpalermo jpalermo left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Looks great. Thanks!

@jpalermo jpalermo merged commit a43a8fc into cloudfoundry:master Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants