Skip to content

Fix slot parsing in unless/onlyif function arguments#53307

Merged
dwoz merged 3 commits intosaltstack:masterfrom
max-arnold:slot-args
Dec 28, 2019
Merged

Fix slot parsing in unless/onlyif function arguments#53307
dwoz merged 3 commits intosaltstack:masterfrom
max-arnold:slot-args

Conversation

@max-arnold
Copy link
Copy Markdown
Contributor

What does this PR do?

I'm trying to improve #51846 (a feature that allows using execution modules in unless/onlyif). I want to be able to do this:

install apache on debian based distros:
  cmd.run:
    - name: make install
    - cwd: /path/to/dir/whatever-2.1.5/
    - unless:
      - fun: file.file_exists
        path: __slot__:salt:test.echo(/usr/local/bin/whatever)

Previous Behavior

Using slots as unless/onlyif function arguments doesn't work.

New Behavior

The basic expectation is that slots should work anywhere within a state.

Tests written?

WIP

Commits signed with GPG?

No

@max-arnold max-arnold force-pushed the slot-args branch 2 times, most recently from 70b5066 to 20e02b4 Compare June 1, 2019 20:12
@max-arnold max-arnold marked this pull request as ready for review June 2, 2019 07:41
@max-arnold max-arnold changed the title [WIP] A naive attempt to parse slots before requisites Fix slot parsing in unless/onlyif function arguments Jun 2, 2019
@max-arnold
Copy link
Copy Markdown
Contributor Author

@DmitryKuzmenko I made a few changes, could you please review this again? Also it would be nice if someone independently verified that my fix works.

My initial one-line patch didn't work because the cdata dictionary does not contain unless/onlyif arguments.

team agreed with me, but we shouldn't move anything existing. We should just add the slot eval to also happen before the unless/onlyif call.

I rewrote the fix as suggested in the Slack room and added a couple of unit tests.

@max-arnold
Copy link
Copy Markdown
Contributor Author

I would also like this to get merged before the Neon feature freeze

@Ch3LL Ch3LL added the Neon label Jun 6, 2019
@Ch3LL Ch3LL changed the base branch from develop to neon June 6, 2019 21:25
@Ch3LL
Copy link
Copy Markdown
Contributor

Ch3LL commented Jun 6, 2019

FYI I migrated this PR from develop to neon to ensure it is included in the upcoming neon release. Let me know if this caused any issues. Thanks

@Ch3LL
Copy link
Copy Markdown
Contributor

Ch3LL commented Jun 7, 2019

I noticed the test_state tests are not running, since develop runs partial tests related to the PR but it did not catch this one. I'm going to try to run a full test run so we can see the results of test_state

@Ch3LL
Copy link
Copy Markdown
Contributor

Ch3LL commented Jun 7, 2019

re-run full all

@Ch3LL Ch3LL self-assigned this Jun 7, 2019
@DmitryKuzmenko
Copy link
Copy Markdown
Contributor

re-run full all

@max-arnold
Copy link
Copy Markdown
Contributor Author

Rebased and added a release note.

@DmitryKuzmenko
Copy link
Copy Markdown
Contributor

@max-arnold thank you for your work. Currently we're working on the upstream tests fixes. After that we'll have to rebase this again and ensure tests are green here. It's just a formal procedure. Sorry for delay.

@dwoz
Copy link
Copy Markdown
Contributor

dwoz commented Dec 19, 2019

@max-arnold can this be re-based against master?

@max-arnold
Copy link
Copy Markdown
Contributor Author

@dwoz Not yet. I'm waiting till #54992 is merged and also trying to make Windows test pass here: #55674

I'll ping you when the PR is ready (hopefully in a day or two)

@max-arnold
Copy link
Copy Markdown
Contributor Author

@dwoz I fixed all the tests and ready to rebase this PR as soon as #54992 is merged

@max-arnold max-arnold changed the base branch from neon to master December 21, 2019 17:01
@max-arnold
Copy link
Copy Markdown
Contributor Author

re-run full centos6-py2
re-run full centos7-py2
re-run full centos7-py2-tcp

@max-arnold
Copy link
Copy Markdown
Contributor Author

re-run full amazon1-py2
re-run full centos6-py2
re-run full ubuntu1604-py2-tcp

@max-arnold
Copy link
Copy Markdown
Contributor Author

@dwoz Rebased, all tests are green!

@dwoz dwoz merged commit bd57aa6 into saltstack:master Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants