-
Notifications
You must be signed in to change notification settings - Fork 5.6k
SmartOS imgadm module and state did not like docker images #48046
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
Conversation
|
I will be super busy the coming week/weekend, but smartos_vmadm should also be updated to not error on docker images. Fixing this is a requirement for smartos_vmadm, I will deal with smartos_vmadm once this is merged and I am less busy. |
|
Lint and test failures seem to be unrelated. |
rallytime
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor documentation requests from me. :)
salt/modules/smartos_imgadm.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be Fluorine, yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
salt/modules/smartos_imgadm.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be Fluorine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
salt/modules/smartos_imgadm.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new function, as well as all of the the other new functions below, should have a versionadded tag for Fluorine. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
For a while now SmartOS has support for docker images, importing one would make smartos_imgadm error. Listing images with docker images installed has the same result.
Like the imgadm module the imgadm states for SmartOS did not handle docker images. With this commit those now work as expected.
|
Done and rebased on develop |
cachedout
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice.
rallytime
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
What does this PR do?
This PR fixes the importing and managing of docker images on SmartOS.
What issues does this PR fix or reference?
N/a
Previous Behavior
Exceptions were encountered when trying to import docker images.
New Behavior
Docker images can now be imported and managed properly.
Tests written?
No
Commits signed with GPG?
No
Backporting
It's a rather large fix so I would prefer to keep it in develop only, although there is no functional change compared to 2018.3.x so it may be worth while pulling this in.