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

Package environment #48021

Merged
merged 71 commits into from Jun 14, 2018
Merged

Package environment #48021

merged 71 commits into from Jun 14, 2018

Conversation

@isbm
Copy link
Contributor

@isbm isbm commented Jun 7, 2018

What does this PR do?

  • Adds feature of configurable environment for the modules
  • Numerous bugfixes for Apt and Yum/Dnf

New Behavior

It is now possible to configure an environment variables for each module and even its functions separately and reuse it with the cmd.run* utility (or other similar matters). This is especially useful if there are package manager plugins that are communicating via environment variables. You find that a lot in Apt/dpkg module. E.g.: write a plugin for a package manager that detects (and records) an event so the package has been updated/installed just directly manually or from within Salt.

Tests written?

Yes

@ghost ghost self-requested a review Jun 7, 2018
meaksh
meaksh approved these changes Jun 7, 2018
Copy link
Member

@meaksh meaksh left a comment

Looks good to me! Thanks for improving this!

@rallytime rallytime requested a review from terminalmage Jun 7, 2018
@@ -1920,3 +1921,45 @@ def output_profile(pr, stats_path='/tmp/stats', stop=False, id_=None):
stacklevel=3
)
return salt.utils.profile.output_profile(pr, stats_path, stop, id_)

Copy link
Collaborator

@terminalmage terminalmage Jun 8, 2018

As noted in the header of this file, we are not adding new functions here. I don't see a natural home in the existing modules under salt/utils/, so this may need to go into its own file. salt/utils/environment.py perhaps?

Copy link
Contributor Author

@isbm isbm Jun 8, 2018

@terminalmage who reads the headers... my bad. But yeah, I thought about exactly this but hesitated. OK, salt/utils/environment.py it is.

FOO: bar
salt.states.pkg:
LC_ALL: en_US.Latin-1
NAME: Fred
Copy link
Collaborator

@terminalmage terminalmage Jun 8, 2018

I'm not sure I like this structure. A lot of users aren't developers, and will thus not intrinsically grok the "module path" naming. From a user-friendliness standpoint, I think it would be more intuitive to do something like:

system-environment:
  modules:
    pkg:
      FOO: bar
  states:
    pkg:
      HELLO: world

@cachedout thoughts on this?

Copy link
Contributor Author

@isbm isbm Jun 8, 2018

@cachedout please agree with the @terminalmage because I like his proposal more than mine. 😉

@isbm isbm changed the title Isbm pkg environ Package environment Jun 8, 2018
@isbm
Copy link
Contributor Author

@isbm isbm commented Jun 8, 2018

@terminalmage Fixed. With even different way:

system-environment:
  modules:
    pkg:
      _:
        FOR: all
      install:
        FOR: install-only
  states:
    pkg:
      _:
        HELLO: world

In some special case, when someone wants to make sure pkg.foo has different stuff than pkg.bar, say a Yum plugin wants to react differently if you installing or upgrading or updating; or Nagios wants to deploy a plugin or fetch it etc.

@isbm
Copy link
Contributor Author

@isbm isbm commented Jun 9, 2018

@rallytime ha, seems like not everything was mocked for Yum/Dnf unit tests! The systemd capability is actually called for real, hence on my side this test has been never failed, but does fail here. Will fix that.

@terminalmage
Copy link
Collaborator

@terminalmage terminalmage commented Jun 11, 2018

@isbm thanks, will re-review on Monday

@isbm
Copy link
Contributor Author

@isbm isbm commented Jun 13, 2018

re-run py

@@ -44,6 +44,7 @@
import salt.utils.systemd
import salt.utils.versions
import salt.utils.yaml
from salt.utils.environment import get_module_environment
Copy link
Collaborator

@terminalmage terminalmage Jun 13, 2018

We either need to use the full module namespace, or we need to import the function as a private function (i.e. as _get_module_environment). The reason for this is because the loader will add all public functions in the module into the __salt__ dunder.

output_loglevel='trace',
python_shell=False,
env={'LC_ALL': 'C', 'LANG': 'C'})
out = _call_apt(cmd, scope=False)
Copy link
Collaborator

@terminalmage terminalmage Jun 13, 2018

This will force a user to set LC_ALL and LANG. Would it be a good idea to set up some sort of default environment? Since we're passing globals() to get_module_environment(), we could maybe define it as a dunder in the global scope?

Copy link
Contributor Author

@isbm isbm Jun 14, 2018

These are just a left-overs from older times, since we're now got run_all properly and resetting the locale there right away.

cmd.extend(args)

params = {'output_loglevel': 'trace',
'ignore_retcode': True,
Copy link
Collaborator

@terminalmage terminalmage Jun 13, 2018

It's rare that we need to do ignore the retcode, I don't think this should be the default. Why not just remove this line and pass through ignore_retcode=True in the kwargs in the two places where we use it (autoremove and list_repo_pkgs)?

Copy link
Contributor Author

@isbm isbm Jun 14, 2018

Yes, that's would do, of course.

@@ -54,6 +54,7 @@
import salt.utils.systemd
import salt.utils.versions
from salt.utils.versions import LooseVersion as _LooseVersion
from salt.utils.environment import get_module_environment
Copy link
Collaborator

@terminalmage terminalmage Jun 13, 2018

Same as with apt, this would add get_module_environment to the __salt__ dunder.

Call yum/dnf.
'''
params = {'output_loglevel': 'trace',
'ignore_retcode': True,
Copy link
Collaborator

@terminalmage terminalmage Jun 13, 2018

Same as with apt, I don't think we should make this the default. Turning this on means we won't get any logging when a command exits with a nonzero exit status. There are instances where this is useful, as some commands return nonzero exit status in non-error cases, but it's an exception, not the norm.

Copy link
Contributor Author

@isbm isbm Jun 14, 2018

Zypper actually doesn't do this. But I just moved it as a default behaviour (most of the time) in yumpkg module. By not ignoring the return code, could it be that the Salt won't succeed anymore running Yum? 😆

@@ -41,6 +41,7 @@
import salt.utils.stringutils
import salt.utils.systemd
from salt.utils.versions import LooseVersion
from salt.utils.environment import get_module_environment
Copy link
Collaborator

@terminalmage terminalmage Jun 13, 2018

Same as apt and yum, this will put get_module_environment in the __salt__ dunder.

@isbm
Copy link
Contributor Author

@isbm isbm commented Jun 14, 2018

@terminalmage terminalmage merged commit 9f5cb35 into saltstack:develop Jun 14, 2018
2 of 9 checks passed
2 of 9 checks passed
codeclimate Code Climate is analyzing this code.
Details
default Build started sha1 is merged.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5750 — RUNNING
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23680 — RUNNING
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10721 — RUNNING
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22643 — RUNNING
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19804 — RUNNING
Details
@wip[bot]
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25953 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants