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

Added compatibility with operations project values #344

Merged
merged 2 commits into from
Jan 17, 2019

Conversation

felix-engelmann
Copy link
Contributor

the LXD Rest api returns an operation id with the query string
?project=default to notify in which project the operation belongs. This
was not sufficiently cut away by .split('/')[-1]
but also needed a subsequent .split('?')[0]

The Mock_lxd has been adapted to test for this feature

In the container.execute the operation_id is needed directly, where it
is fixed too

Signed-off-by: Felix Engelmann fe-github@nlogn.org

the LXD Rest api returns an operation id with the query string
?project=default to notify in which project the operation belongs. This
was not sufficiently cut away by .split('/')[-1]
but also needed a subsequent .split('?')[0]

The Mock_lxd has been adapted to test for this feature

In the container.execute the operation_id is needed directly, where it
is fixed too

Signed-off-by: Felix Engelmann <fe-github@nlogn.org>
@codecov-io
Copy link

codecov-io commented Dec 13, 2018

Codecov Report

Merging #344 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
+ Coverage   96.83%   96.84%   +0.01%     
==========================================
  Files          12       12              
  Lines         980      984       +4     
  Branches      110      109       -1     
==========================================
+ Hits          949      953       +4     
  Misses         12       12              
  Partials       19       19
Impacted Files Coverage Δ
pylxd/models/container.py 91.49% <100%> (+0.03%) ⬆️
pylxd/models/operation.py 96.77% <100%> (+0.34%) ⬆️

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 ad07da9...346e558. Read the comment docs.

@ajkavanagh
Copy link
Contributor

This is nice; however, I do have a concern. I'm guessing that the ?project=... bit only exists on LXD instances that have the projects API extension? In that case, this ought to check to see if that extension exists and only then try to take that bit off. I also think that split() is a bit of a blunt tool here, and it would be better to try to match it and then remove it. Possibly using urlparse.parse_qs might be a way of doing it in a way that pulls out the relevant parts?

import urllib.parse
urllib.parse.parse_qs("https://hello.com/the/part?hello=one&project=10")

{'https://hello.com/the/part?hello': ['one'], 'project': ['10']}

Signed-off-by: Felix Engelmann <fe-github@nlogn.org>
@felix-engelmann
Copy link
Contributor Author

I agree that split is NOT the right tool. Unfortunately it is used throughout the hole project :(
"asdf".split('?')[0] would still give the correct result, even if the projects extension is not enabled, but I think the urlparse option is way more pythonic

@ajkavanagh
Copy link
Contributor

Hey @felix-engelmann, yes, I agree split is used everywhere! :) I'm not one of the original maintainers, but only the most recent. I like to try an encourage a bit of "code gardening" to see if we can improve things, and sometimes take it a bit far!! Sorry if the request looked a bit strange in the context of the rest of the code.

My thinking went a bit like this: the original code used a split('/') and then grabbed the last one, which I thought was kind of okay. But then adding the querystring params to it suddenly upped the complexity, and I reckoned "I bet urlparse has a function to do this that also errors out, etc.". So it was the slight bump in complexity that pushed me to think - "I wonder if we can improve this little bit of code and make the library a bit more bulletproof". I hope that's okay.

@felix-engelmann
Copy link
Contributor Author

felix-engelmann commented Dec 15, 2018

How do you like my static method in Operation to extract the operation id?

And in general, I saw that you are planning a major revision for version 3. Do you want a complete rewrite? Because then it would make little sense to put a lot of effort into this version.

Copy link
Contributor

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

This is good. Thanks!

@ajkavanagh
Copy link
Contributor

How do you like my static method in Operation to extract the operation id?

Yes, that's good.

And in general, I saw that you are planning a major revision for version 3. Do you want a complete rewrite? Because then it would make little sense to put a lot of effort into this version.

It's not going to be a complete re-write, but perhaps a different set of API calls. i.e. this version will continue for all the code that relies on it.

A lot of the 'smart' code in the library will probably be re-used, but it'll take less of a 'object orientated' approach, and more of a functional approach; i.e. removing the 'sync' function, and thus respecting that the contents of a returned 'object' are only valid at the point that it is returned. So, no 'modify and object and save it', rather "send the changes as per the API".

However, there's not much urgency around a version 3.0, and so version 2.0 is still where it is at :) It's more my dream of how it might be better.

@ajkavanagh ajkavanagh merged commit 7897af7 into canonical:master Jan 17, 2019
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.

3 participants