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

chore: Remove storage and volume interface implementation #1972

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

aayushrangwala
Copy link
Contributor

@aayushrangwala aayushrangwala commented Nov 12, 2023

Changes Title (replace this with a logical title for your changes)

Remove unused and unsupported apis from equinix metal

Description

  • Remove implementation of storage APIs as they were removed from Packet years ago. there is no direct replacement. Equinix Metal offers Pure and NetApp storage devices today but there is not equivalent functionality as far as the Equinix Metal API is concerned with directly attaching storage to servers and equinix metal doesnot supports only 2 storage providers and

For more information on contributing, please see Contributing
section of our documentation.

Status

Ready For Review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

@aayushrangwala aayushrangwala force-pushed the equinix-metal-remove-storage-ifc branch from c9fada1 to 86ff77e Compare November 12, 2023 09:44
@aayushrangwala aayushrangwala changed the title Remove storage and volume interface implementation chore: Remove storage and volume interface implementation Nov 12, 2023
@displague
Copy link

Reviewer note: The Elastic Block storage feature was removed from Equinix Metal in June 2021.
https://web.archive.org/web/20210620005004/https://metal.equinix.com/developers/docs/storage/elastic-block-storage/#elastic-block-storage

@aayushrangwala aayushrangwala marked this pull request as ready for review November 15, 2023 09:36
@displague
Copy link

displague commented Nov 15, 2023

@Kami is this something that you can review and merge? There are three Equinix Metal PRs up. I have reviewed them on their Equinix Metal merits.

@Kami
Copy link
Member

Kami commented Dec 3, 2023

Thanks for the contribution.

Since this is a breaking change, can you please add an entry in upgrade notes file (docs/upgrade_notes.rst)? Thanks.

@codecov-commenter
Copy link

Codecov Report

Merging #1972 (86ff77e) into trunk (dfbb595) will increase coverage by 0.06%.
Report is 10 commits behind head on trunk.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##            trunk    #1972      +/-   ##
==========================================
+ Coverage   83.20%   83.25%   +0.06%     
==========================================
  Files         353      353              
  Lines       81423    81252     -171     
  Branches     8591     8559      -32     
==========================================
- Hits        67742    67645      -97     
+ Misses      10874    10820      -54     
+ Partials     2807     2787      -20     
Files Coverage Δ
libcloud/compute/drivers/equinixmetal.py 70.54% <ø> (+6.18%) ⬆️
libcloud/test/compute/test_equinixmetal.py 92.89% <ø> (+2.76%) ⬆️

@Kami
Copy link
Member

Kami commented Dec 3, 2023

@aayushrangwala Can you please sync up this branch with a latest trunk? I wanted to wrap it up myself locally, but I noticed more work is needed to make this change complete - it appears create_node() still calls create_volume() / attach_volume(), etc.

Please let me know when that has been addressed and when all checks are passing and I will have a look again.

On that note, it would also be good to update PR description with some context and why this change is needed (I know that context may be available in other PR, but we should also update this PR description).

Thanks.

Signed-off-by: Ayush Rangwala <ayush.rangwala@gmail.com>
@aayushrangwala aayushrangwala force-pushed the equinix-metal-remove-storage-ifc branch from 86ff77e to e8786ee Compare January 17, 2024 17:15
@aayushrangwala
Copy link
Contributor Author

@Kami can you please review again, Ive rebased and updated.

@@ -570,6 +511,8 @@ def _metal_v1_storage_attachments_2c16a96f_bb4f_471b_8e2e_b5820b9e1603(
if method == "DELETE":
return (httplib.NO_CONTENT, "", {}, httplib.responses[httplib.NO_CONTENT])

=======

Choose a reason for hiding this comment

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

merge conflict should be cleaned up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

Signed-off-by: Ayush Rangwala <ayush.rangwala@gmail.com>
Copy link

@displague displague left a comment

Choose a reason for hiding this comment

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

removals look right.

@Kami Kami added this to the v3.9.0 milestone Apr 18, 2024
@Kami
Copy link
Member

Kami commented Apr 18, 2024

@antoinebourayne I had a look and it looks like this comment is still relevant - #1972 (comment)

I see create_node() method is still calling volume related methods which have been removed. create_node() should be updated to remove those calls and disk + disk_size argument should be removed as well (since it doesn't make sense / it's unused without corresponding volume management methods).

asfgit pushed a commit that referenced this pull request Apr 18, 2024
asfgit pushed a commit that referenced this pull request Apr 18, 2024
asfgit pushed a commit that referenced this pull request Apr 18, 2024
@asfgit asfgit merged commit d0b9f90 into apache:trunk Apr 18, 2024
@Kami
Copy link
Member

Kami commented Apr 18, 2024

I made and pushed the following changes myself:

  • Remove disk and disk_size argument from create_node method - daddbae
  • Add upgrade notes entry - 70a5f87
  • Remove volumes from list_resources_async() - fc48cac
  • Fix lint - 84b68be

With those changes, the code has now also been merged into trunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants