Skip to content

[master] Add missing 'fun' for returned events from "wfunc" SSH executions#54935

Merged
dwoz merged 14 commits into
saltstack:masterfrom
meaksh:master-fix-missing-fun-in-ssh-for-wfuncs
Aug 24, 2020
Merged

[master] Add missing 'fun' for returned events from "wfunc" SSH executions#54935
dwoz merged 14 commits into
saltstack:masterfrom
meaksh:master-fix-missing-fun-in-ssh-for-wfuncs

Conversation

@meaksh
Copy link
Copy Markdown
Contributor

@meaksh meaksh commented Oct 9, 2019

What does this PR do?

This PR fixes an issue when executing wfuncs, for example, state.show_highstate over salt-ssh.

While the state.show_highstate response is successfully received, the event that is fired to the "EventBus" is missing the fun attribute.

This PR will add the fun to the fired event in case this is missing.

Tests written?

Yes

Commits signed with GPG?

Yes

@meaksh
Copy link
Copy Markdown
Contributor Author

meaksh commented Oct 9, 2019

I'll work on the tests ASAP

@meaksh meaksh force-pushed the master-fix-missing-fun-in-ssh-for-wfuncs branch from 2667230 to 07ea53f Compare October 9, 2019 14:19
Comment thread salt/client/ssh/__init__.py Outdated
Copy link
Copy Markdown
Contributor

@isbm isbm Oct 9, 2019

Choose a reason for hiding this comment

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

This can be just written as:

data.setdefault("fun", fun)

But I am still not sure this is the right place of doing it. In handle_ssh the function might not appear due to various reasons there as well as the next element is just a text, so the data was just created above. In that case I would rather find out why fun key did not appeared at all and if it is a bug, then should be done in the mentioned method instead.

Maybe @thatch45 might help here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm following here the same approach that for id: in case it doesn't exist we explicitly add it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks right since this data is only part of the event it should not interfere with any other aspect of execution. Using data.setdefault("fun", fun) would also be an acceptable approach but it is not something that I am too concerned with here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My main concern is why sometimes "fun" and "Id" in this case needs to be checked. Shouldn't we rather move that above where "data" variable is redefined from string type to dict instead? I mean:

if isinstance(...
    data = {"id": ...., "fun":...

Instead of double-checking.

@meaksh meaksh force-pushed the master-fix-missing-fun-in-ssh-for-wfuncs branch from 07ea53f to d78bb19 Compare December 10, 2019 12:17
@meaksh meaksh requested a review from a team as a code owner December 10, 2019 12:17
@ghost ghost requested a review from Ch3LL December 10, 2019 12:17
@meaksh meaksh changed the title Add missing 'fun' for returned events from "wfunc" SSH executions [master] Add missing 'fun' for returned events from "wfunc" SSH executions Dec 10, 2019
@meaksh meaksh force-pushed the master-fix-missing-fun-in-ssh-for-wfuncs branch from 363925e to 8aef6dd Compare April 7, 2020 12:43
@Ch3LL Ch3LL removed the request for review from a team April 15, 2020 14:39
@dwoz dwoz added the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Apr 26, 2020
@meaksh
Copy link
Copy Markdown
Contributor Author

meaksh commented May 11, 2020

@dwoz added missing unit tests

@dwoz dwoz added has-failing-test and removed needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases labels May 12, 2020
Ch3LL
Ch3LL previously approved these changes Aug 5, 2020
@Ch3LL
Copy link
Copy Markdown
Contributor

Ch3LL commented Aug 5, 2020

@meaksh looks like this needs a rebase and theres some failing test although not certain they are related to your change.

@meaksh
Copy link
Copy Markdown
Contributor Author

meaksh commented Aug 6, 2020

@Ch3LL I've updated the branch with latest changes from master. Let see how tests are looking now 👍

@Ch3LL
Copy link
Copy Markdown
Contributor

Ch3LL commented Aug 7, 2020

looks like pre-commit is failing and that should do it :)

@meaksh
Copy link
Copy Markdown
Contributor Author

meaksh commented Aug 11, 2020

@Ch3LL done! 👍

Now it looks correct apart from the ci/lint failure which I think it's spurious in this case.

@dwoz dwoz merged commit 2fe8286 into saltstack:master Aug 24, 2020
@meaksh meaksh deleted the master-fix-missing-fun-in-ssh-for-wfuncs branch August 25, 2020 07:42
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Magnesium Mg release after Na prior to Al

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants