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

file.managed calculates changes twice on normal runs #36588

Closed
slinn0 opened this issue Sep 27, 2016 · 3 comments
Closed

file.managed calculates changes twice on normal runs #36588

slinn0 opened this issue Sep 27, 2016 · 3 comments
Labels
Bug broken, incorrect, or confusing behavior fixed-pls-verify fix is linked, bug author to confirm fix P3 Priority 3 Platform Relates to OS, containers, platform-based utilities like FS, system based apps severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone

Comments

@slinn0
Copy link
Contributor

slinn0 commented Sep 27, 2016

Description of Issue

file.managed renders jinja and calculates the diffs between existing file and source file twice. This is suboptimal and file.check_managed_changes should only run when test=True

Setup

Was running a file.managed with a jinja template and noticed that jinja templating was being called twice for every file.managed.

Steps to Reproduce Issue

Make a file.managed with a jinja template and have the jinja code log something and notice it is called twice.

Versions Report

Noticed bug appeared when upgrading from 2015.8 to 2016.3. Verified bug exists in todays develop branch. I have a fix for develop and 2016.3. Will make a PR shortly.

@cachedout cachedout added the fixed-pls-verify fix is linked, bug author to confirm fix label Sep 27, 2016
@cachedout
Copy link
Contributor

@slinn0 Thanks for sending this and for sending a fix along with it. We love it when that happens. 👍

@slinn0
Copy link
Contributor Author

slinn0 commented Sep 27, 2016

@cachedout,

I tried to cherry pick this fix into 2016.3 which is the branch we are using and there are conflicts because of changes in surrounding code. I have a fix for 2016.3 that is tested. Should I make a PR against 2016.3 too?

@rallytime
Copy link
Contributor

@slinn0 That would be great!

@rallytime rallytime added Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around P3 Priority 3 Platform Relates to OS, containers, platform-based utilities like FS, system based apps labels Sep 27, 2016
@rallytime rallytime added this to the Approved milestone Sep 27, 2016
msiebeneicher pushed a commit to msiebeneicher/salt that referenced this issue Sep 30, 2016
* 'develop' of https://github.com/saltstack/salt:
  Fix traceback when checking command whitelist/blacklist
  tests.integration.__init__: except psutil error
  archive.extracted: Use `user`/`group`, not `archive_user`
  Better merge conflict fix: pass **params to consul.Consul in consul_pillar.py
  Add consul host, port, and token values back in with conf.get()
  Minor: syntax error fixes.
  Improve and fix `_check_cache_minions`
  Backport of PR saltstack#36589 / Issue saltstack#36588 to 2016.3 branch.
  Add support for ACL Tokens in consul_pillar with the option consul.token
  Add version_cmp for FreeBSD pkg.
gitebra pushed a commit to gitebra/salt that referenced this issue Sep 30, 2016
* upstream/develop: (21 commits)
  Workaround for interference between loader test and yaml test
  timezone: Don't assume /etc/timezone exists
  Updated rabbitmq module unit test
  Port the 'rabbitmq' module to Windows
  Reworking wording in the last line and some whitespace
  Fix traceback when checking command whitelist/blacklist
  tests.integration.__init__: except psutil error
  Missed that the correct link gets created; yay sphinxdoc
  Adding quotes to help identify this is a document title; would link if this was not a man page
  Making an attempt to clarify a few things in the salt.fileserver.gitfs section
  Update index.rst
  Copying block about storing SLS files
  Trivial orchestrate doc changes
  archive.extracted: Use `user`/`group`, not `archive_user`
  Better merge conflict fix: pass **params to consul.Consul in consul_pillar.py
  Add consul host, port, and token values back in with conf.get()
  Minor: syntax error fixes.
  Improve and fix `_check_cache_minions`
  Backport of PR saltstack#36589 / Issue saltstack#36588 to 2016.3 branch.
  Add support for ACL Tokens in consul_pillar with the option consul.token
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior fixed-pls-verify fix is linked, bug author to confirm fix P3 Priority 3 Platform Relates to OS, containers, platform-based utilities like FS, system based apps severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

No branches or pull requests

3 participants