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.serialize does not merge lists even if merge_if_exists: True is set #66903

Open
2 of 9 tasks
ikselven opened this issue Sep 18, 2024 · 2 comments
Open
2 of 9 tasks
Labels
Bug broken, incorrect, or confusing behavior needs-triage

Comments

@ikselven
Copy link

Description
I am using file.serialize to update the contents of a JSON file. Since that file may already exist, I have set merge_if_exists: True in order to merge the file contents with the dataset given in the state. Now if both the file and dataset have a list, Salt does not merge the lists but instead overwrites it with version from dataset.

Setup
(Please provide relevant configs and/or SLS files (be sure to remove sensitive info. There is no general set-up of Salt.)

Please be as specific as possible and give set-up details.

  • on-prem machine
  • VM (Virtualbox, KVM, etc. please specify)
  • VM running on a cloud service, please be explicit and add details
  • container (Kubernetes, Docker, containerd, etc. please specify)
  • or a combination, please be explicit
  • jails if it is FreeBSD
  • classic packaging
  • onedir packaging
  • used bootstrap to install

Steps to Reproduce the behavior
(Include debug logs if possible and relevant)

Create following 3 files:

9d2538af0da3:/ # cat /srv/salt/top.sls
base:
  '*':
    - bar

9d2538af0da3:/ # cat /srv/salt/bar.sls
write_test_file:
  file.serialize:
    - name: /test.json
    - dataset:
        eric: bacon
        john:
          - eggs
          - spam
    - serializer: json
    - merge_if_exists: True
    - show_changes: True

9d2538af0da3:/ # cat /test.json
{
  "terry": [
    "spam",
    "bacon"
  ],
  "john": [
    "beans"
  ]
}

Then apply the state or run test-mode:

9d2538af0da3:/ # salt-call --local state.apply bar test=True
local:
----------
          ID: write_test_file
    Function: file.serialize
        Name: /test.json
      Result: None
     Comment: Dataset will be serialized and stored into /test.json
     Started: 15:51:31.413810
    Duration: 17.361 ms
     Changes:
              ----------
              diff:
                  ----------
                  eric:
                      ----------
                      new:
                          bacon
                      old:
                          <_null_>
                  john:
                      ----------
                      new:
                          - eggs
                          - spam
                      old:
                          - beans

Summary for local
------------
Succeeded: 1 (unchanged=1, changed=1)
Failed:    0
------------
Total states run:     1
Total run time:  17.361 ms

Expected behavior

The lists from the file and from dataset should be merged, like this:

9d2538af0da3:/ # salt-call --local state.apply bar test=True
local:
----------
          ID: write_test_file
    Function: file.serialize
        Name: /test.json
      Result: None
     Comment: Dataset will be serialized and stored into /test.json
     Started: 15:53:02.186886
    Duration: 17.415 ms
     Changes:
              ----------
              diff:
                  ----------
                  eric:
                      ----------
                      new:
                          bacon
                      old:
                          <_null_>
                  john:
                      ----------
                      new:
                          - beans
                          - eggs
                          - spam
                      old:
                          - beans

Summary for local
------------
Succeeded: 1 (unchanged=1, changed=1)
Failed:    0
------------
Total states run:     1
Total run time:  17.415 ms

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)
9d2538af0da3:/ # salt-call --versions-report
Salt Version:
          Salt: 3006.0

Python Version:
        Python: 3.11.9 (main, Apr 08 2024, 06:18:15) [GCC]

Dependency Versions:
          cffi: 1.17.0
      cherrypy: Not Installed
   contextvars: 2.4
      dateutil: Not Installed
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.4
       libgit2: Not Installed
  looseversion: 1.3.0
      M2Crypto: 0.42.0
          Mako: Not Installed
       msgpack: 1.1.0
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     packaging: 24.1
     pycparser: 2.22
      pycrypto: Not Installed
  pycryptodome: Not Installed
        pygit2: Not Installed
  python-gnupg: Not Installed
        PyYAML: 6.0.1
         PyZMQ: 25.1.2
        relenv: Not Installed
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.5

System Versions:
          dist: opensuse-tumbleweed 20240917 n/a
        locale: utf-8
       machine: x86_64
       release: 6.10.10-arch1-1
        system: Linux
       version: openSUSE Tumbleweed 20240917 n/a

Additional context

The issue seems to be a missing third parameter merge_lists=True in this function call:

                merged_data = salt.utils.dictupdate.merge_recurse(
                    existing_data, dataset
                )

in

merged_data = salt.utils.dictupdate.merge_recurse(

@ikselven ikselven added Bug broken, incorrect, or confusing behavior needs-triage labels Sep 18, 2024
Copy link

welcome bot commented Sep 18, 2024

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@bdrx312
Copy link
Contributor

bdrx312 commented Sep 19, 2024

I think the behavior may be working as intended. I don't think it should be merging lists by default or at least should be optional to not merge lists. Maybe merge_if_exists should accept different merge options instead of just a boolean True/False.

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 needs-triage
Projects
None yet
Development

No branches or pull requests

2 participants