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

Updated Linode (Akamai Connected Cloud) support (including cloud-init) #1946

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

mraygalaxy2
Copy link
Contributor

@mraygalaxy2 mraygalaxy2 commented Aug 25, 2023

Updated Linode (Akamai Connected Cloud) support (including cloud-init)

Description

  1. cloud-init support is a new feature available in 2023.
  2. The main entry point (create_node) had an arrangement of non-keyword parameters that were not consistent with other libcloud drivers. This has been fixed.
  3. One remaining function (already available in the API) was exposed to also be consistent with other drivers.

Status

  • done, 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)

@Kami
Copy link
Member

Kami commented Aug 27, 2023

Thanks for the contribution.

When you get a chance, can you please add some test case for those changes? Thanks.

@Kami
Copy link
Member

Kami commented Aug 27, 2023

It also seems like that it may be a good idea to rename the driver in the future (Akamai Connected Cloud)?

We can still leave (deprecated) "linode" alias in place for the foreseeable future to make the change backward compatible.

@mraygalaxy2
Copy link
Contributor Author

Acknowledged @Kami . I'll work on that.

Regarding the name change, we haven't been directed to do that right now by the company, particularly as the API endpoint (api.linode.com) is unable to change for the forseeable future.

Copy link

@Dorthu Dorthu left a comment

Choose a reason for hiding this comment

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

Ran though all of the changes and tested them against the API and Metadata Service, everything worked like I expected it to 👍

@mraygalaxy2
Copy link
Contributor Author

okie dokie @Kami I've pushed those tests. Take a 2nd look?

@mraygalaxy2
Copy link
Contributor Author

NOTE: I do want to remove (or cleanup) this comment: d8add44#diff-e2e26eb0ef50c4cf2bf6b12658cd644d7ca1fe06cd2a3bc307d607149c83a2a4R976

Looking for feedback before I do that.

# image=None,
# name=None,
#
# Comments welcome on how backwards compatibility (if any) should work here.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to leave it as-in and document this backward incompatible / breaking change in the upgrade notes file (https://github.com/apache/libcloud/blob/trunk/docs/upgrade_notes.rst), ideally with some concrete before / after code examples.

@Kami
Copy link
Member

Kami commented Sep 13, 2023

@mraygalaxy2 Thanks for adding the tests and addressing some very old API inconsistency in the create_node() method.

I think it would be good to document that breaking API change in the upgrade notes file (d8add44#r127271125).

Besides that, LGTM.

@mraygalaxy2
Copy link
Contributor Author

@Kami Thank you, sir. I'll push a change right now to update that document.

@@ -997,6 +1038,12 @@ def create_node(
:keyword ex_private_ip: whether or not to request a private IP
:type ex_private_ip: ``bool``

:keyword ex_userdata: add cloud-config compatible userdata to be
Copy link
Member

Choose a reason for hiding this comment

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

I think it would actually be better to let user pass in a raw string and then we can take care of base64 encoding the string inside the method ourselves.

This way the method abstracts away this implementation detail from the end user (we already do a similar thing in a bunch of places).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dorthu Would you like me to make that change? ^^^

Copy link

Choose a reason for hiding this comment

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

I agree that it would be more convenient for the library to handle the encoding, especially if there's precedent elsewhere in libcloud, as long as that behavior is featured prominently in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do! Thanks guys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okie dokie @Kami @Dorthu I have pushed that change (and tested it against Linode).

@Kami
Copy link
Member

Kami commented Sep 13, 2023

@mraygalaxy2 Thanks. There is one more small thing which I thought I already commented on in the past (https://github.com/apache/libcloud/pull/1946/files#r1324941487), but maybe I forgot to :)

@mraygalaxy2 mraygalaxy2 force-pushed the linode-cloudinit branch 2 times, most recently from 1a8cbc9 to bf2f681 Compare September 13, 2023 21:56
1. cloud-init support is a new feature available in 2023.
2. The main entry point (create_node) had an arrangement of non-keyword parameters that were not consistent with other libcloud drivers. This has been fixed.
3. One remaining function (already available in the API) was exposed to also be consistent with other drivers.
@mraygalaxy2
Copy link
Contributor Author

mraygalaxy2 commented Sep 18, 2023

OK, base64 changes complete. Also tell me if those those docs/upgrade_notes.rst and CHANGES.rst changes look ok. I'm happy to clean them up if there's a mistake.

@asfgit asfgit merged commit 03e60cc into apache:trunk Sep 20, 2023
@Kami Kami modified the milestones: v3.8.0, v3.9.0 Sep 20, 2023
@Kami
Copy link
Member

Kami commented Sep 20, 2023

Thanks for addressing the review feedback.

I made some small changes and merged it into trunk:

  • Reword upgrade notes entry a bit - 01c7e61
  • Default ex_userdata to None instead of False since this makes more sense for an argument which is not a boolean - bb0dba7
  • Add test case for ex_userdata argument - bb0dba7

@mraygalaxy2
Copy link
Contributor Author

Thank you so much, @Kami for the changes! Much appreciated.

mraygalaxy pushed a commit to ibmcb/cbtool that referenced this pull request Sep 20, 2023
1. Merged: apache/libcloud#1946
   (This will likely go into version 3.9.0, based on their current
    release schedule)
2. The use of metadata changed, so make cloudbench change accordingly.
maugustosilva pushed a commit to ibmcb/cbtool that referenced this pull request Sep 25, 2023
1. Merged: apache/libcloud#1946
   (This will likely go into version 3.9.0, based on their current
    release schedule)
2. The use of metadata changed, so make cloudbench change accordingly.
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.

4 participants