Skip to content

Fixes: #4875 monkeypatch st2 sensor earlier#4976

Merged
punkrokk merged 13 commits intomasterfrom
monkey_patch_sensorcontainer_sooner
Jul 16, 2020
Merged

Fixes: #4875 monkeypatch st2 sensor earlier#4976
punkrokk merged 13 commits intomasterfrom
monkey_patch_sensorcontainer_sooner

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

punkrokk commented Jun 26, 2020

@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

guzzijones commented Jun 29, 2020

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.

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

punkrokk commented Jul 16, 2020

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

@nmaludy nmaludy Awaiting requested review from nmaludy

+1 more reviewer

@arm4b arm4b arm4b approved these changes

Reviewers whose approvals may not affect merge requirements

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

Development

Successfully merging this pull request may close these issues.

Using boto3 in a python3 sensor gives a RecursionError when building boto sensor Connection to mongo db over SSL doesn't work

5 participants

@punkrokk @guzzijones @Kami @arm4b @cognifloyd