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

Add proposal for version 3.0 of the pylxd API #283

Closed

Conversation

ajkavanagh
Copy link
Contributor

This is a proposal/spec to explain what a v3 of the pylxd API would be
like and why it is needed. This would be a breaking change for clients
of pylxd.

It's opened for discussion here; thoughts both for and against are welcomed.

This is a proposal/spec to explain what a v3 of the pylxd API would be
like and why it is needed.  This would be a breaking change for clients
of pylxd.

2. The attribute system only tries to do a single level of JSON loading, and
doesn't load the whole tree of metadata returned in an API call. Thus you
get the (slightly) perverse situation of some attributes container JSON
Copy link
Contributor

Choose a reason for hiding this comment

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

should this read: some attributes contain JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should. Thanks for the catch.


.. code-block:: python

container = c.profiles['default'].used_by
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be containers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, c at this point is a container object, and thus this would be correct. I'll check though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, and concur, that c is a container; I'm concerned that, on asking for the profile, that we would not then have the list of containers using that profile (ex: default has many containers)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think what I may not have explained well is that used_by would be a ContainerManager instance, meaning that it represents 0..n containers. I'll change the initial variable assignment to container_manager and add some words.

container = c.profiles['default'].used_by
<... Manager for containers ...>

i.e. the ``used_by`` property would return a manager to access profiles.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that this manager would access containers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And, yes, this is true too.

@codecov-io
Copy link

Codecov Report

Merging #283 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #283   +/-   ##
=======================================
  Coverage   97.25%   97.25%           
=======================================
  Files          11       11           
  Lines         728      728           
  Branches       84       84           
=======================================
  Hits          708      708           
  Misses          6        6           
  Partials       14       14

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 d76bb3d...1ab11d8. 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