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

[BUG] file.comment incorrectly reports changes in test mode #61662

Closed
OrangeDog opened this issue Feb 17, 2022 · 1 comment · Fixed by #62045
Closed

[BUG] file.comment incorrectly reports changes in test mode #61662

OrangeDog opened this issue Feb 17, 2022 · 1 comment · Fixed by #62045
Assignees
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around State-Module
Milestone

Comments

@OrangeDog
Copy link
Contributor

OrangeDog commented Feb 17, 2022

Description
When running a file.comment state the second time, no changes are made.
However, when running in test mode, it reports that it will make changes.

Setup

/etc/elasticsearch/jvm.options:
  file.comment:
    - regex: '9-:-Xlog:gc.*'
    - watch_in:
      - service: elasticsearch

Steps to Reproduce the behavior

$ sudo tail -n2 /etc/elasticsearch/jvm.options
# JDK 9+ GC logging
#9-:-Xlog:gc*,gc+age=trace,safepoint:file=/var/log/elasticsearch/gc.log:utctime,pid,tags:filecount=32,filesize=64m
$ sudo salt-call state.apply --state-output=changes
local:

Summary for local
--------------
Succeeded: 226
Failed:      0
--------------
Total states run:     226
Total run time:    26.433 s
$ sudo salt-call state.test --state-output=changes
local:
----------
          ID: /etc/elasticsearch/jvm.options
    Function: file.comment
      Result: None
     Comment: File /etc/elasticsearch/jvm.options is set to be updated
     Started: 10:58:22.192903
    Duration: 2.929 ms
     Changes:
              ----------
              /etc/elasticsearch/jvm.options:
                  updated
----------
          ID: elasticsearch_service
    Function: service.running
        Name: elasticsearch
      Result: None
     Comment: Service is set to be restarted
     Started: 10:58:22.287999
    Duration: 29.613 ms
     Changes:

Summary for local
--------------
Succeeded: 226 (unchanged=2, changed=1)
Failed:      0
--------------
Total states run:     226
Total run time:    24.190 s

Expected behavior
No changes, same as when not running in test mode.

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)
Salt Version:
          Salt: 3004

Dependency Versions:
          cffi: 1.14.5
      cherrypy: Not Installed
      dateutil: 2.7.3
     docker-py: Not Installed
         gitdb: 2.0.6
     gitpython: 3.0.7
        Jinja2: 2.10.1
       libgit2: 1.3.0
      M2Crypto: 0.31.0
          Mako: Not Installed
       msgpack: 0.6.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: 2.20
      pycrypto: Not Installed
  pycryptodome: 3.6.1
        pygit2: 1.7.1
        Python: 3.8.10 (default, Nov 26 2021, 20:14:08)
  python-gnupg: 0.4.5
        PyYAML: 5.3.1
         PyZMQ: 18.1.1
         smmap: 2.0.5
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.2

System Versions:
          dist: ubuntu 20.04 focal
        locale: utf-8
       machine: x86_64
       release: 5.4.0-99-generic
        system: Linux
       version: Ubuntu 20.04 focal
@OrangeDog OrangeDog added Bug broken, incorrect, or confusing behavior needs-triage labels Feb 17, 2022
@OrangeDog OrangeDog added severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around State-Module and removed needs-triage labels Feb 17, 2022
@OrangeDog OrangeDog added this to the Approved milestone Feb 17, 2022
@waynew
Copy link
Contributor

waynew commented Feb 17, 2022

Oh yeah, this is uh... interesting. The file.comment module was actually deprecated back in 0.17.0, but never removed. The file.comment state uses its own custom regex, while it then calls file.search (which uses file.replace) to know if it should change it or not. But then it uses the file.comment state, which uses a different set of behaviors.

It should probably just use file.replace - once with dry run for test, and once without to actually make the changes 🙃

nicholasmhughes added a commit to nicholasmhughes/salt that referenced this issue May 10, 2022
garethgreenaway pushed a commit to garethgreenaway/salt that referenced this issue May 23, 2022
Ch3LL pushed a commit that referenced this issue May 26, 2022
* Making the retry state system feature available when parallel is set to True.

* swapping out using time.sleep Salt function for Python time.sleep.

* Skipping parallel tests on Windows.

* Fixing items, values, and keys functions in the data module.  Moving integration to functional tests.  Converting unit test over to pytest.

* adding changleog.

* fixes #62044 add ignore_missing to file.comment state

* fixes #61662 fix file.comment reports changes in test mode

* add 61662.fixed changelog

* add final test mode run after comment is successful

* fix uncomment_regex

* fix uncomment_regex

* Try not to trigger the GLIBC race condition.

See https://sourceware.org/bugzilla/show_bug.cgi?id=19329

Fixes #62071

Signed-off-by: Pedro Algarvio <[email protected]>

* Add missing CLI example to ``network.fqdns``

Signed-off-by: Pedro Algarvio <[email protected]>

* Sleep between submitting new jobs to thread pool

Signed-off-by: Pedro Algarvio <[email protected]>

* Sleep before trying to resolve instead

Signed-off-by: Pedro Algarvio <[email protected]>

* Shorter sleeps

* fixes #61944 fixed backslash literal bytestring

* When states are running in parallel, ensure that the total run time produced by the highstate outputter takes that into account.

* removing unrelated change

* Adding changelog.

* Skip parallel test on Windows.

* skip pdbedit unit tests on ubuntu 2204 because md4 is not supported as a hash type

* refresh db at the beginning for destructive aptpkg functional tests

* fix failing test_pkgrepo_managed_absent and actually capture and keep signed-by information in salt/modules/aptpkg.py custom implementation of apt repository management

* Update tests/unit/modules/test_pdbedit.py

Co-authored-by: Pedro Algarvio <[email protected]>

* Disable hack to force allowing forking on macOS

* Use the newer nox platforms file which installs Py3.9

Signed-off-by: Pedro Algarvio <[email protected]>

* Support macOS defaulting to spawn as the default multiprocessing method

* Remove unused attribute

* Switch to ``salt.utils.platform.spawning_platform()``

Signed-off-by: Pedro Algarvio <[email protected]>

* Reconstruct client instances on spawning platforms

* Also skip this test for now on macOS

Signed-off-by: Pedro Algarvio <[email protected]>

* Reconstruct the ``State`` class on spawned processes.

Signed-off-by: Pedro Algarvio <[email protected]>

* Force a reference to ``salt.ext.tornado.iostream.StreamClosedError`` to avoid ``NameError`` issues

Signed-off-by: Pedro Algarvio <[email protected]>

* Skip, for now, problematic tests on spawning platforms

Signed-off-by: Pedro Algarvio <[email protected]>

* DeltaProxy minions do not work on spawning platforms

Signed-off-by: Pedro Algarvio <[email protected]>

* Avoid a ``NameError`` when failing to initialize the process list

Signed-off-by: Pedro Algarvio <[email protected]>

* Use Python 3.9 on macOS

Signed-off-by: Pedro Algarvio <[email protected]>

* Drop requirements on Darwin for Py<3.9

Signed-off-by: Pedro Algarvio <[email protected]>

* Same test run timeout as windows

Signed-off-by: Pedro Algarvio <[email protected]>

* Don't shell out! Access the database directly and properly escape values.

Signed-off-by: Pedro Algarvio <[email protected]>

* Fixes for the new macOS Catalina and BigSur CICD images

Signed-off-by: Pedro Algarvio <[email protected]>

* Migrate ``integration.modules.test_mac_pkgutil`` to PyTest and functional tests.

Fixes #60819

Signed-off-by: Pedro Algarvio <[email protected]>

* Add changelog for #57742

Fixes #57742

Signed-off-by: Pedro Algarvio <[email protected]>

* Add changelog entry.

Fixes #55847

Signed-off-by: Pedro Algarvio <[email protected]>

* initial Proxy Module

* inital pass of restconf states

* adding codeauthor

* adding restconf module

* doc update

* doc update

* doc update

* add requirements chefk to states module

* doc update

* doc update

* doc update

* add changelog entry

* Update salt/proxy/restconf.py

Co-authored-by: Wayne Werner <[email protected]>

* Update salt/proxy/restconf.py

Co-authored-by: Wayne Werner <[email protected]>

* remove noqa statements

* docs update & opts defaults

* docs update

* error checks + https transport method

* docs update

* docs update

* doing a fix for commit check

* adding test and updating modules

* removing old code

* DRY

* docs update

* output updates

* prepping for deepdiff switch

* states testing

* more tests

* changed changelog from fixed to added

* force pre-commit checks

* new style of diff output that is readable

* updating tests and output

* updating output style to make YAML diff more readable

* add test for restconf module

* adjusting based on pr review

* adjusting based on pr review

* adjusting true/false verify based on pr review

* change uri to path and clean args list

Co-authored-by: Wayne Werner <[email protected]>

* adjusting uri to path based on pr review

* adjusting capabilities path based on pr review

* Update salt/modules/restconf.py

Co-authored-by: Wayne Werner <[email protected]>

* adjusting val based on pr review

* caps to confirm to RFC 8040

* WIP: refactor based on feedback

* WIP: changed results to be any kind of falsey val

* linting - pre-commit

* updating docs

* Update salt/states/restconf.py

Co-authored-by: Wayne Werner <[email protected]>

* updating style

* fixes for lint and nox pass

* fixing

* blacken

* Update restconf.py

* add cli examples

* change logging type

* add pytest skip for ordereddict issue with legacy python

* add sys library for pytest mark skipif check

* pre-commit fixes

* Update salt/proxy/restconf.py

Co-authored-by: Pedro Algarvio <[email protected]>

* Update salt/states/restconf.py

Co-authored-by: Pedro Algarvio <[email protected]>

* Update tests/pytests/unit/modules/test_restconf.py

Co-authored-by: Pedro Algarvio <[email protected]>

* Update tests/pytests/unit/states/test_restconf.py

Co-authored-by: Pedro Algarvio <[email protected]>

* Update tests/pytests/unit/proxy/test_restconf.py

Co-authored-by: Pedro Algarvio <[email protected]>

* pre-commit fix

* update logging

* Update restconf.py

* some rest apis dont follow the standard correctly (cisco) and need another header

* fixes #61946 sync_after_install immutabledict error

* add configurable sync sleep for create

* add testing around the particular sync command called

* Adds the ability to get file version information on Windows

- Adds file.version that will get just the version
- Adds file.version_details to get additional information
- Adds tests
- Adds changelog

@amalaguti originally submitted a PR for this: #59770, but he has
since deleted his branch. Recognizing his contribution here.

* Update states.chef for version 16.x and 17.x Chef Infra Client output

* Support previous Chef version plus version 16.x and 17.x output.

* Remove legacy chef unit test

* Add unit tests for chef state

These tests cover the entire state, but are only concerned with the
ret["result"] of the outputs. It would be a good idea to test other
outputs as well - these tests could be modified to include assertions
about the other parts of ret. (Obviously they would need to be slightly
renamed. Maybe to `..._return_expected_ret`

* Update salt-bootstrap to 2022.03.15 release

* Update state.py

Fixing lint.

* Running pre-commit manually.

* swap out salt.utils.platform.is_windows for salt.utils.platform.spawning_platform.  Bump up duration for test_retry_option_success_parallel to 30 seconds to account for test running on Mac OS.

Co-authored-by: Megan Wilhite <[email protected]>
Co-authored-by: nicholasmhughes <[email protected]>
Co-authored-by: Pedro Algarvio <[email protected]>
Co-authored-by: emmadionne1 <[email protected]>
Co-authored-by: MKLeb <[email protected]>
Co-authored-by: Caleb Beard <[email protected]>
Co-authored-by: Pedro Algarvio <[email protected]>
Co-authored-by: Jamie Murphy <[email protected]>
Co-authored-by: Jamie (Bear) Murphy <[email protected]>
Co-authored-by: Jamie Murphy <[email protected]>
Co-authored-by: Wayne Werner <[email protected]>
Co-authored-by: Cassandra Durnford <[email protected]>
Co-authored-by: Jamie Murphy <[email protected]>
Co-authored-by: Jamie (Bear) Murphy <[email protected]>
Co-authored-by: Twangboy <[email protected]>
Co-authored-by: Eric Ham <[email protected]>
Co-authored-by: Wayne Werner <[email protected]>
Co-authored-by: krionbsd <[email protected]>
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 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around State-Module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants