Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
  • 16:08
    FabioRosado labeled #1935
  • 16:08
    FabioRosado unlabeled #1935
  • 16:08

    FabioRosado on master

    Add CLI option to print config … (compare)

  • 16:08
    FabioRosado closed #1935
  • Jul 01 18:58
    FabioRosado commented #1905
  • Jul 01 16:08
    jacobtomlinson labeled #1905
  • Jul 01 15:10
    rodrigo-albuquerque commented #1905
  • Jun 29 14:19
    rodrigo-albuquerque commented #1936
  • Jun 29 09:30
    rodrigo-albuquerque commented #1936
  • Jun 29 09:29
    rodrigo-albuquerque commented #1936
  • Jun 28 16:30
    jacobtomlinson commented #1936
  • Jun 28 13:37
    rodrigo-albuquerque commented #1936
  • Jun 28 13:36
    rodrigo-albuquerque commented #1936
  • Jun 28 13:35
    rodrigo-albuquerque commented #1936
  • Jun 28 12:03
    codecov[bot] commented #1936
  • Jun 28 12:01
    codecov[bot] commented #1936
  • Jun 28 12:00
    codecov[bot] commented #1936
  • Jun 28 11:59
    codecov[bot] commented #1936
  • Jun 28 11:59
    codecov[bot] commented #1936
  • Jun 28 11:53
    rodrigo-albuquerque edited #1936
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
I was struggling to use VCRpy or our request mocking because some of the URLs include a randomised UUID which changes every time! WHyyyyyyy!
FabioRosado
@fabiorosado:matrix.org
[m]

That seems like a great Microsoft idea 😄

We were struggling with getting logs from azure because Microsoft decided to make one part of the storage url random 🤦‍♂️

Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
It's such a PITA!
FabioRosado
@fabiorosado:matrix.org
[m]
But maybe they did implement some useless thing like again I. Azure it seems that they take a screen shot of the file in storage. Because when you are storing text files you really need a screenshot with the first logs 🤣
All the urls have that UUID?
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
It seems like when creating a message with Teams you POST to a URL with a randomised uuid on the end, that UUID becomes the UUID of the message. But it's the local SDK that created the UUID, which is a bit weird IMO. usually you would just post to some generic endpoint and the server side creates the UUID and returns it in the response.
So using VCR to record call fails because that URL changes every time you run the tests. I guess I'll need to implement a custom handler, but thats just annoying.
FabioRosado
@fabiorosado:matrix.org
[m]
Oh I see that sucks
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
Yup
cognifloyd (Jacob Floyd)
@cognifloyd:matrix.org
[m]
I replied to this in the Teams PR. I think you could add a pytest fixture that generates a consistent list of UUIDs.
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
👍
dud1337
@dud1337:138.io
[m]

Ajit D'Sa and I have been working on some DB updates. Updated MongoDB, added a postgres, but also we could update SQLite.

The main reason is this:
Skills should be allowed to specify which table they save to.

Right now, one opsdroid instance with one DB, two different skills will save into the same table/collection of the same db.

So we allow:

  1. setting default table name in DB
  2. depending on skill, allowing default table (or tables) name in skill config
  3. setting table name in the put and get functions as an optional kwarg.

I do not know how to write good tests for this. Is there a way to spin up a postgres instance on commit and run unit tests against it? Or decent mock modules available?

Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]

Awesome thanks! I'm excited to see this work happening.

I'm curious about the desire to have different tables/collections for each skill. I've always considered it a feature that the opsdroid memory object is accessible from any skill. Each skill can just choose a prefix for the key name if there are concerns about collision.

1 reply
For testing you might want to look into pytest-docker to start a postgres database as part of the pytest fixtures.
1 reply
I think it would be good if you could open an issue to discuss the table per skill idea. Then we can keep all the conversation about it there.
1 reply
dud1337
@dud1337:138.io
[m]
Cadair
@cadair:cadair.com
[m]
which we have this far achieved with a context manager
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
I just want to take a minute to highlight that we have a community code of conduct. Everyone engaging in discussion anywhere in the opsdroid community is expected to follow these standards. I am super excited by the recent surge in activity and having folks improve opsdroid. Let's make this this is a fun and positive environment for everyone. https://github.com/opsdroid/opsdroid/blob/master/CODE_OF_CONDUCT.md
dud1337
@dud1337:138.io
[m]
(my apologies for the bluntness)
1 reply
Sleuth
@sloth56:chat.mountainview.theworkpc.com
[m]
It's really easy to get someone defensive when people start asking for changes in what they have been working on for a while now. So in this case it's not entirely on dud here. Not to bring this down that path but just wanted to bring that up.
Jacob Tomlinson
@jacobtomlinson:matrix.org
[m]
I hadn't targeted that at anyone in particular. There has just been a general vibe for a while lately that I wanted to get ahead of.
Sleuth
@sloth56:chat.mountainview.theworkpc.com
[m]
Understandable
Jacob Tomlinson
@jacobtomlinson:matrix.org
[m]
I'm aware that I can be a little blunt in my reviews, so it's a good reminder for myself too.
Ajit D'Sa
@ajitdsa:matrix.org
[m]
I just want to say that I appreciate all you guys, the time you take and spend on this, your feedback, and your opinions. So far I feel that all of it comes from a good place, and that I also appreciate Jacob Tomlinson's efforts to make sure it stays that way. Thanks to all of you.
Sleuth
@sloth56:chat.mountainview.theworkpc.com
[m]
Agreed. This is an amazing project!
Jacob Tomlinson
@jacobtomlinson:matrix.org
[m]
The Teams PR is finally green! 🎉 opsdroid/opsdroid#1679
cognifloyd (Jacob Floyd)
@cognifloyd:matrix.org
[m]
Woohoo! I just merged the teams PR! Thank you Jacob Tomlinson and FabioRosado for figuring out how to add teams support!
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
Thanks for all the help reviewing. And your work on the test machinery. Tests have been the main blocker for the PR (and time) but that was a joy to test.
Ajit D'Sa
@ajitdsa:matrix.org
[m]
Maybe I'm missing something, but if I change this line https://github.com/opsdroid/opsdroid/blob/master/opsdroid/connector/github/tests/test_connector_github.py#L237 to assert event.abc == "pyup-bot" and then run pytest opsdroid/connector/github/tests/test_connector_github.py::test_pr_merged it still passes. Any reason why this would be?
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
Oooh interesting. I think because the assertions are inside the skill they get ignored.
We log exceptions in skills and move on. Don't want user code to cause the whole thing to die. https://github.com/opsdroid/opsdroid/blob/a45490d1bdceca39b49880e20262b55ea0be101d/opsdroid/core.py#L466
So we should probably be asserting that Exception when running skill is not logged once the skill has been run.
Good catch thanks!
Ajit D'Sa
@ajitdsa:matrix.org
[m]
Yes, that totally makes sense. Hmmmm, I understand this, but in terms of implementation, that's like saying we should put the call_endpoint in a try and then catch the exception coming back, yah? If not, lemme know if you can pseudocode it out for me, and I'll try making the change
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
Ajit D'Sa
@ajitdsa:matrix.org
[m]
Oho. Okay, I'll see if I can get that working for you. Thanks for much for the pointer.... learning :)
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
👍
We probably need to do this in every test that has assertions inside the skill
Ajit D'Sa
@ajitdsa:matrix.org
[m]
Yup. If I get it working for one, I'll try to do it everywhere :)
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
Awesome
Ajit D'Sa
@ajitdsa:matrix.org
[m]
Jacob Tomlinson: so I am not convinced the skill-based tests are working. Here is what I mean by this... I couldn't get the caplog stuff working. But I read all about caplog and there's no reason it shouldn't work. So I tried just straight raising an exception in the skill, and finally just tried putting a breakpoint (like a pdb.set_trace()) in the skill... and none of it registered. There didn't seem to be any change I could make in the skill that would affect anything at all. I tried this with multiple skills. I only tried this with skills in the github connector so far.
Can you (or anyone for that matter) give this a try and see if you are getting something different from me? Maybe I'm doing something wrong.
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
I think you're right. I'm not sure the skills themselves ever get executed. Opsdroid adds the skill to the event loop and returns a 201 to let you know the execution has been created. But it doesn't wait for the execution to finish, this is because users could put all sorts in their skill and if it takes too long the GitHub webhook will time out.
Really we need something after the assert resp.status == 201 which waits for the skill to be called. Maybe even adding an asyncio.sleep after will be enough to get it to hit your breakpoint. But we probably need some testing utility which blocks until the skill has been called.
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
Looks like there was a bug in the GitHub connector which did mean the skill was never called. I've opened opsdroid/opsdroid#1808 to fix it. I also updated the test to check that the skill was called.
Jacob Tomlinson (Slack)
@_slack_opsdroid_U5MK1BW83:matrix.org
[m]
With that bug fixed things are as we thought. The assertions in the skill fail but opsdroid continues anyway and the test passes. Checking the log and asserting that no exception was raised fixes that, so if you break the assertion the test fails. https://github.com/opsdroid/opsdroid/pull/1808/commits/d8dd73b9a70ba6198cc460740f17373f8f39a3aa
Ajit D'Sa
@ajitdsa:matrix.org
[m]
Yesssss, awesome. Good stuff! Also, glad it wasn’t something deeper as I had originally suspected. Will officially review shortly :)
Ajit D'Sa
@ajitdsa:matrix.org
[m]
Jacob Tomlinson: your PR appears ready to merge 😃