Skip to content

WIP: feat: add userdata api to SDK #17

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

Closed
wants to merge 2 commits into from

Conversation

abarbare
Copy link
Contributor

@abarbare abarbare commented Aug 2, 2018

Hello!
This PR implement the reading of user_data information from the SDK. As this feature need to call the base_url with a privileged port, I had to override the python requests object.

Regarding the unit tests, I didn't find an elegant way to integrate them except mocking get get_userdata method and compare the result to a string (which is basically comparing two static strings). If you have any piece of advice about it, I would be more than happy to integrate them.

@brmzkw
Copy link
Contributor

brmzkw commented Aug 2, 2018

Shouldn't this code belong to the MetadataAPI object instead?

False, return a dictionary.
"""
with requests.Session() as s:
s.mount('http://', SourcePortAdapter(random.randrange(1, 1024)))
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we should loop from 1 to 1024 and catch errors instead of using random.randrange() which could raise (even if it's unlikely). Using random could cause inconsistent results.

Copy link
Contributor Author

@abarbare abarbare Aug 2, 2018

Choose a reason for hiding this comment

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

With a loop, how do you handle the multiple runs of the SDK? As when you query the API with a specific port, this port is "block" and the socket remains open for a few minutes. Using a random.randrange copy the functioning of the curl --local-port 1-1024 http://169.254.42.42/user_data command.


if as_shell:
return s.get('%s/user_data' % self.base_url).text
return s.get('%s/user_data?format=json' % self.base_url).text

Choose a reason for hiding this comment

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

You might want to return a dict here, not .text


super(UserDataAPI, self).__init__(auth_token=None, **kwargs)

def get_userdata(self, as_shell=False):

Choose a reason for hiding this comment

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

Actually this calls returns the list of userdata keys, not the values.

@abarbare abarbare changed the title feat: add userdata api to SDK WIP: feat: add userdata api to SDK Aug 2, 2018
@abarbare
Copy link
Contributor Author

abarbare commented Aug 2, 2018

How do you want to handle the multiline values for a key with the as_shell option?
/conf for dict instance generate a line with a header followed by the values

LOCATION='PLATFORM_ID HYPERVISOR_ID NODE_ID CLUSTER_ID ZONE_ID'
LOCATION_PLATFORM_ID=XX
LOCATION_HYPERVISOR_ID=XX
LOCATION_NODE_ID=XX
LOCATION_CLUSTER_ID=XX
LOCATION_ZONE_ID=par1

@codecov-io
Copy link

codecov-io commented Aug 2, 2018

Codecov Report

Merging #17 into develop will decrease coverage by 100%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #17     +/-   ##
=======================================
- Coverage      100%    0%   -100%     
=======================================
  Files            4     4             
  Lines           83   117     +34     
  Branches        15    19      +4     
=======================================
- Hits            83     0     -83     
- Misses           0   117    +117
Impacted Files Coverage Δ
scaleway/apis/api_metadata.py 0% <0%> (-100%) ⬇️
scaleway/apis/api_compute.py 0% <0%> (-100%) ⬇️
scaleway/apis/api_billing.py 0% <0%> (-100%) ⬇️
scaleway/apis/api_account.py 0% <0%> (-100%) ⬇️

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 1b656b8...100760f. Read the comment docs.

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