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

Use tgt_type instead of expr_form #186

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

the-glu
Copy link

@the-glu the-glu commented Apr 2, 2019

expr_form was deprecated in 2017.07 and removed in latest salt release (2019.02).

There was a partial fix in #143, but that didn't fix low api calls - right now pepper and this option cannot be used again a 2019.02 server.

This PR fix this, but without any backward compatibility.

Copy link
Contributor

@gtmanfred gtmanfred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we could also have some regression testing here, that would be greatly appreciated.

Thanks,
Daniel

pepper/libpepper.py Show resolved Hide resolved
@kovacsbalu
Copy link

Hi, I got error with salt 2018.3.2 (Oxygen) using client_batch and expr_form.

salt_api.low([{'tgt': '*', 'batch': '1', 'client': 'local_batch', 'arg': 'sleep 5; date', 'fun': 'cmd.run', 'expr_form': 'glob'}]
... )
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/user/.local/lib/python2.7/site-packages/pepper/libpepper.py", line 298, in low
    return self.req(path, lowstate)
  File "/home/user/.local/lib/python2.7/site-packages/pepper/libpepper.py", line 242, in req
    raise PepperException('Server error.')
pepper.exceptions.PepperException: Server error.

With tgt_type it works perfectly.

@nicholasmhughes
Copy link
Collaborator

Howdy! 👋

Apologies for the long delay on this integrating this code. We certainly appreciate your effort!

Pepper is getting some renewed focus and love, and we're checking in to see if you'd like to update this against the latest version of the base branch and ensure that all merge conditions are met.

If the items addressed by this contribution have already been fixed/added, are no longer an issue, or you no longer have a desire to work on this... please let us know and/or close out this PR.

If we don't hear from you within 7 days, we'll assume the code has been abandoned and close it out. If you'd like to revisit this in the future, you can always open a new PR.

Thank you, and we hope to see you active in the Salt ecosystem in the future!

@enricotagliani
Copy link

I'm not the author of this PR but I can confirm that this fix is required for interacting with recent releases of Salt.
Thanks!

@the-glu
Copy link
Author

the-glu commented Apr 20, 2023

I seem I missed the comments two years ago, sorry about that.

I will update the PR with comment as soon as possible, probably next week :)

@the-glu
Copy link
Author

the-glu commented Apr 24, 2023

Hello,

I added backward compatiblity + some tests, however I'm not sure how to tests test - there is no Travis CI there ?

@barneysowood
Copy link
Collaborator

Hello,

I added backward compatiblity + some tests, however I'm not sure how to tests test - there is no Travis CI there ?

Thanks for the updates and adding tests. We're currently moving the tests to Github Actions (see #226 and #227), but we're waiting on some help from vmware folks to get privs to alter the PR workflow. I'll let you know once that's sorted - thanks for our patience.

@barneysowood
Copy link
Collaborator

Hello,
I added backward compatiblity + some tests, however I'm not sure how to tests test - there is no Travis CI there ?

Thanks for the updates and adding tests. We're currently moving the tests to Github Actions (see #226 and #227), but we're waiting on some help from vmware folks to get privs to alter the PR workflow. I'll let you know once that's sorted - thanks for our patience.

@the-glu - just to let you know that I've now merged the updated test suite that will use GH Actions. Could you update your branch?

A brief look at your tests suggests they should still work without any changes, but if you have any issues please let me know - happy to help out.

@the-glu
Copy link
Author

the-glu commented May 2, 2023

@the-glu - just to let you know that I've now merged the updated test suite that will use GH Actions. Could you update your branch?

A brief look at your tests suggests they should still work without any changes, but if you have any issues please let me know - happy to help out.

@barneysowood Done, but it does seem you have to approve the actions :)

@the-glu
Copy link
Author

the-glu commented May 2, 2023

Ok it does seem to works, I fix tests asap

@barneysowood
Copy link
Collaborator

barneysowood commented May 2, 2023 via email

@the-glu the-glu force-pushed the tgt_type branch 2 times, most recently from 767b67f to 802f1ec Compare May 3, 2023 08:55
@the-glu
Copy link
Author

the-glu commented May 3, 2023

Ok I fixed tests, PR should be ok now :)

@os-sengel
Copy link

Hi @barneysowood,
is there any chance this will be merged soon? This PR fixes a major incompatibility with the current version of salt

@timwsuqld
Copy link

@barneysowood not having this merged is preventing us from using pepper currently

@nicholasmhughes nicholasmhughes linked an issue Nov 3, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to pass tgt_type to API
8 participants