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

Fix slot parsing in unless/onlyif function arguments #53307

Merged
merged 3 commits into from
Dec 28, 2019

Conversation

max-arnold
Copy link
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
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
Contributor Author

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

@Ch3LL Ch3LL added the ZRELEASED - Neon retired label label Jun 6, 2019
@Ch3LL Ch3LL changed the base branch from develop to neon June 6, 2019 21:25
@Ch3LL
Copy link
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
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
Contributor

Ch3LL commented Jun 7, 2019

re-run full all

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

re-run full all

@max-arnold
Copy link
Contributor Author

Rebased and added a release note.

@DmitryKuzmenko
Copy link
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
Contributor

dwoz commented Dec 19, 2019

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

@max-arnold
Copy link
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
Contributor Author

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

@max-arnold
Copy link
Contributor Author

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

@max-arnold
Copy link
Contributor Author

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

@max-arnold
Copy link
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