move kwarg parsing to api backend instead of in pepper #174
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@mattp- tests /should/ be fixed now |
Codecov Report
@@ Coverage Diff @@
## develop #174 +/- ##
=========================================
- Coverage 72.39% 71.1% -1.3%
=========================================
Files 5 5
Lines 594 609 +15
Branches 126 131 +5
=========================================
+ Hits 430 433 +3
- Misses 137 146 +9
- Partials 27 30 +3 |
gtmanfred
suggested changes
Dec 1, 2018
| @@ -231,6 +233,10 @@ def req(self, path, data=None): | |||
| if (self.debug_http): | |||
| logger.debug('Response: %s', content) | |||
| ret = json.loads(content) | |||
|
|
|||
| if not self.salt_version and 'x-salt-version' in f.headers: | |||
| self._parse_salt_version(f.headers['x-salt-version']) | |||
There was a problem hiding this comment.
Can you write some mocked unittests that test this feature out?
rather than attempting to deconstruct args locally to fully reproduce the proper low state to pass to the api, we instead leave the kwargs as is. The reason for this is the RunnerClient will deconstruct and deserialize nested yaml args. We could equivalently do that locally but it seems pepper tries to be a no-dependency dist so we just hand off the work instead. This PR partially depends on release of saltstack/salt#50124 to determine if a release is neon or newer; however, we can safely assume that if the salt_version header is not provided in a request it is a salt-api older than that. Fixes saltstack#57
I originally tried making this work as a pytest parameterized fixture, but couldn't get things working properly with pytest-salt daemon mgmt. If someone figures that out feel free to revert this tox level knob.
was being applied pepper.cli before which is useless for pepper.libpepper etc.
minions.connected doesnt take an arg
failed output isnt returned if nothing failed anymore. additionally, returned as valid json
make this act more reasonable. we drop the x-auth-token header when using /run as the idea of that endpoint is to use whats in the lowstate. we get rid of the /token route handling as it seemingly does the same thing as /login, and doesnt actually exist in the rest_tornado impl so I'm considering it deprecated for simplicity.
this seems to do the right thing in more scenarios.
This reverts commit 59c013a. this fixes tornado but breaks cherrypy - I mistakenly assumed the payload token returned on login was the real eauth token not the session.id. I'll add a route to /token in tornado that is equivalent to /login on tornado to mimick backcompat behavior even though they are equivalent there due to not having a proxy session store.
auth should be handled only by token provided in low
runner output should be identical now
both codepaths are valid in both cherrypy and tornado. bunch of stuff xfail'd for now in tornado as there are pending develop/ PR's to salt to make things fully cross-compatible.
this will be used by tornado to know how long to persist sync calls for instead of the app_default_timeout. Also seems like a reasonable thing to do anyway.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
rather than attempting to deconstruct args locally to fully reproduce
the proper low state to pass to the api, we instead leave the kwargs as
is. The reason for this is the RunnerClient will deconstruct and
deserialize nested yaml args. We could equivalently do that locally but
it seems pepper tries to be a no-dependency dist so we just hand off the
work instead.
This PR partially depends on release of saltstack/salt#50124
to determine if a release is neon or newer; however, we can safely assume that if
the salt_version header is not provided in a request it is a salt-api
older than that.
Fixes #57