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

Support for virtual execution module pkg on AIX #48924

Merged
merged 9 commits into from Aug 8, 2018

Conversation

Projects
None yet
4 participants
@dmurphy18
Copy link
Contributor

commented Aug 3, 2018

What does this PR do?

Added support for virtual execution module pkg on AIX,
provides support for pkg.install, pkg.remove, pkg.list_pkgs, pkg.version

Also adds support for pkg.is_installed

Adds ability to install/remove IBM AIX filesets and/or rpm packages for the AIX platform

What issues does this PR fix or reference?

saltstack/zh#1408

Previous Behavior

There was no support for execution module pkg on AIX

New Behavior

pkg.install, pkg.remove, pkg.list_pkgs, pkg.version, pkg.is_installed are now functional on AIX

Tests written?

No - Tests coming lates

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Aug 3, 2018

@rallytime rallytime added the Fluorine label Aug 3, 2018

@rallytime rallytime requested review from cachedout and terminalmage Aug 3, 2018

@terminalmage
Copy link
Contributor

left a comment

This is missing functions like latest_version. Please refer to our package provider writing documentation here: https://docs.saltstack.com/en/latest/topics/development/package_providers.html#package-functions

@dmurphy18

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@terminalmage I know, talked with Kent Besaw about the additional functionality, since he talks with the customer weekly (that requested this support) and agreed to limit to the functionality requested rather than adding extra's not originally requested. So pulled that out: but can add it and others back in to provide barest minimum support to meet Fluorine commitments. Can add latest_available and upgrade_available (not too applicable in AIX since no repositories as in the sense of Linux or even Solaris, and the latest is that already committed to the database AIX keeps - ODM). Can add the functions and have them return what is already there.

Decision was to add the additional functionality after Fluorine shipped.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

I don't find that to be an acceptable solution, since pkg states expect at least some of the missing functions to exist and will throw a KeyError if you run them and they're not.

@dmurphy18

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@terminalmage I was going to check the states pkg to see what was affected when writing the unit tests. I shall have another talk with Kent and raise the issues again with him, to push for the other functionality.

@dmurphy18

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@terminalmage talked with Kent and adding latest_version and upgrade_available. Others shall be added in another PR, to ensure this meets deadlines

@dmurphy18 dmurphy18 force-pushed the dmurphy18:aix_pkg branch from c61cc98 to f750a38 Aug 3, 2018

@dmurphy18

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@terminalmage Added support for pkg.latest_version, pkg.upgrade_available similar to Solaris 10 which also does not support repositories.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2018

salt '*' pkg.latest_version <package name>
salt '*' pkg.latest_version <package1> <package2> <package3> ...
NOTE: As package repositories are not presently supported for AIX

This comment has been minimized.

Copy link
@terminalmage

terminalmage Aug 6, 2018

Contributor

"package repositories" is redundant as it results in the word "package" appearing twice in the same clause. Also, the 2nd appearance should be "packages", not "package". Thus, this should be:

    NOTE: As repositories are not presently supported for AIX installp/RPM
    packages, this function will always return an empty string for a given
    package.

This comment has been minimized.

Copy link
@dmurphy18

dmurphy18 Aug 6, 2018

Author Contributor

@terminalmage I hate getting nailed by cut and paste, :(, will fix.

@dmurphy18

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

@rallytime Those pylint errors are fixed.

@dmurphy18 dmurphy18 force-pushed the dmurphy18:aix_pkg branch from 4cbada8 to 6a08687 Aug 6, 2018

@dmurphy18

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

@rallytime Can Mike be pulled as a reviewer since he is PTO and a continent and an ocean away ?

if __grains__['os_family'] == 'AIX':
return __virtualname__
return (False,
'The aixpkg execution module failed to load.')

This comment has been minimized.

Copy link
@cachedout

cachedout Aug 6, 2018

Collaborator

I don't think this is a good error message. failed suggests that it should have loaded and did not. Perhaps something like Did not load AIX module on non-AIX OS?

This comment has been minimized.

Copy link
@dmurphy18

dmurphy18 Aug 6, 2018

Author Contributor

Updated message, was following error messages currently used for other pkg execution modules.

return __salt__['cmd.retcode'](cmd) == 0


def install(name=None, refresh=False, pkgs=None, version=None, test=False, **kwargs):

This comment has been minimized.

Copy link
@cachedout

cachedout Aug 6, 2018

Collaborator

version and test doesn't appear to be documented below.

This comment has been minimized.

Copy link
@dmurphy18

dmurphy18 Aug 6, 2018

Author Contributor

@cachedout Will update to include refresh, version and test.

David Murphy

@rallytime rallytime changed the base branch from develop to fluorine Aug 6, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

@dmurphy18 I have changed the base branch of this PR from develop to fluorine for inclusion in the Fluorine feature release.

@rallytime rallytime requested a review from terminalmage Aug 6, 2018

@rallytime rallytime merged commit 0403a6d into saltstack:fluorine Aug 8, 2018

3 of 9 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
codeclimate 15 issues to fix
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.