Skip to content

Datasource for UpCloud#743

Merged
TheRealFalcon merged 20 commits into
canonical:masterfrom
ajmyyra:datasource-upcloud
Feb 8, 2021
Merged

Datasource for UpCloud#743
TheRealFalcon merged 20 commits into
canonical:masterfrom
ajmyyra:datasource-upcloud

Conversation

@ajmyyra
Copy link
Copy Markdown
Contributor

@ajmyyra ajmyyra commented Dec 28, 2020

Datasource for UpCloud

New datasource utilizing UpCloud metadata API, including relevant unit tests and documentation.

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@ajmyyra
Copy link
Copy Markdown
Contributor Author

ajmyyra commented Dec 28, 2020

CLA has been signed by UpCloud as an entity and sent to contributor-agreement@canonical.com on 18th of this month. No response yet, which isn't that surprising with holidays and end of year ongoing. :) Additionally, I added myself as a contributor in #742. Please let me know if there are additional steps needed for this, and of course if the PR itself is lacking something.

Comment thread cloudinit/sources/helpers/upcloud.py
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Hi @ajmyyra . Overall this submission looks good. Along with my inline comments, I have two additional comments:

  1. You'll need to update tools/ds-identify as well. That would involve adding to DI_DSLIST_DEFAULT, add a dscheck_UpCloud() (yours should look similar to DigitalOcean, and add a unit test to tests/unittests/test_ds_identify.py
  2. In response to your exchange with OddBloke, I think yes, we'd prefer to use a v2 network config (for the reasons he listed) if it's not too much of a burden.

Comment thread cloudinit/sources/helpers/upcloud.py Outdated
Comment thread cloudinit/sources/helpers/upcloud.py Outdated
Comment thread doc/rtd/topics/datasources/upcloud.rst Outdated
@ajmyyra
Copy link
Copy Markdown
Contributor Author

ajmyyra commented Jan 12, 2021

Thank you for the review! I'll get these done later this week. Changing to v2 network config isn't too much work, so I'll gladly do it at the same time.

Comment thread cloudinit/sources/helpers/upcloud.py
@ajmyyra ajmyyra force-pushed the datasource-upcloud branch from 37c04a2 to 03aa632 Compare January 28, 2021 14:04
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

This looks good to me now. I'll wait to see if @smoser has any additional comments, but if not it should be good to merge.

Comment thread doc/rtd/topics/datasources/upcloud.rst Outdated
Comment thread cloudinit/sources/DataSourceUpCloud.py
Copy link
Copy Markdown
Collaborator

@smoser smoser left a comment

Choose a reason for hiding this comment

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

It does look fine to me. I'll approve. you can address my comments if you like... You should probably fix the retries comment if its wrong though.

Copy link
Copy Markdown
Collaborator

@smoser smoser left a comment

Choose a reason for hiding this comment

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

meant to hit 'approve'.

@TheRealFalcon TheRealFalcon merged commit 0497c7b into canonical:master Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants