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.

Sign up for GitHub

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

Jump to bottom

Fixes: #4875 monkeypatch st2 sensor earlier #4976

Merged
merged 13 commits into from
Jul 16, 2020
Merged

Fixes: #4875 monkeypatch st2 sensor earlier #4976

merged 13 commits into from
Jul 16, 2020

Conversation

punkrokk
Copy link
Member

@punkrokk punkrokk commented Jun 26, 2020 •
edited by arm4b
Loading

Fixes: #4975
Fixes: #4832

@punkrokk punkrokk added the bug label Jun 26, 2020
@punkrokk punkrokk added this to the 3.2.1 milestone Jun 26, 2020
@punkrokk punkrokk requested review from Kami, arm4b and nmaludy June 26, 2020 04:05
@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Jun 26, 2020
@punkrokk punkrokk mentioned this pull request Jun 26, 2020
Closed
@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Jun 26, 2020
@punkrokk
Copy link
Member Author

@alexku7 Could you test this branch out and tell me if it resolves #4832 for you?

@guzzijones
Copy link
Contributor

guzzijones commented Jun 26, 2020 •
edited
Loading

Yeah monkey patching should always be the very first thing. Before imports. Awesome work

cognifloyd
cognifloyd reviewed Jun 29, 2020
View reviewed changes
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

This looks good to me. monkey_patch() should definitely come before imports wherever required. There is one typo in a comment.

@guzzijones
Copy link
Contributor

It looks like these are all in the cmd folder so they are command scripts that are run by for services. This way of monkey patching should work just fine. And yes it needs to be before any other imports. If we have other cases of monkey patching we need to do the same.
I don't see any of these scripts used as imports at first glance which is what I thought would be an issue.

Kami
Kami approved these changes Jul 1, 2020
View reviewed changes
Copy link
Member

@Kami Kami left a comment

Choose a reason for hiding this comment

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

LGTM, good catch (we had a couple of similar issues like that in the past) 👍

Any chance we could some how add a test case for it?

@punkrokk
Copy link
Member Author

punkrokk commented Jul 1, 2020

@Kami Do you have any suggestions for how to add a test case for this?

The only thing I can think of is adding one that fails if we see a recursion error?

arm4b
arm4b reviewed Jul 16, 2020
View reviewed changes
arm4b
arm4b reviewed Jul 16, 2020
View reviewed changes
arm4b
arm4b approved these changes Jul 16, 2020
View reviewed changes
Copy link
Member

@arm4b arm4b left a comment •
edited
Loading

Choose a reason for hiding this comment

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

💯

Tests are nice, but I'm good if we merge it as is, because it's tricky to test it with monkey-patch thing added.

@arm4b
Update CHANGELOG.rst
8789cbc 
Co-authored-by: Eugen C. <stackstorm@armab.io>
JP Bourget and others added 2 commits July 16, 2020 11:00
@punkrokk punkrokk mentioned this pull request Jul 16, 2020
Closed
11 tasks
@punkrokk
Copy link
Member Author

Forum thread: https://forum.stackstorm.com/t/centos-8-problems-with-rabbitmq-connections-when-sensor-dispatches-trigger/1328/12

@punkrokk punkrokk merged commit 11470e6 into master Jul 16, 2020
@punkrokk punkrokk deleted the monkey_patch_sensorcontainer_sooner branch July 16, 2020 16:23
@punkrokk punkrokk modified the milestones: 3.2.1, 3.3.0 Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cognifloyd cognifloyd cognifloyd left review comments

@Kami Kami Kami approved these changes

@arm4b arm4b arm4b approved these changes

@nmaludy nmaludy Awaiting requested review from nmaludy

Assignees
No one assigned
Labels
bug size/M PR that changes 30-99 lines. Good size to review.
Projects
None yet
Milestone
3.3.0
5 participants
@punkrokk @guzzijones @Kami @arm4b @cognifloyd