Skip to content

Conversation

@Rados13
Copy link
Contributor

@Rados13 Rados13 commented Oct 16, 2023

@Rados13 Rados13 marked this pull request as ready for review October 20, 2023 10:43
@roznawsk roznawsk removed their request for review October 23, 2023 11:56
Copy link
Contributor

@mickel8 mickel8 left a comment

Choose a reason for hiding this comment

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

Could we add an option to easily run tests using Jellyfish that runs locally? Something like mix test.local?

@Rados13 Rados13 requested review from LVala and mickel8 October 24, 2023 06:10
@Rados13 Rados13 requested a review from LVala October 24, 2023 09:26
steps:
- checkout
- run: docker compose -f docker-compose-test.yaml run test
- run: docker compose -f docker-compose-test.yaml up test
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH we don't even need to add test at the end. Simply docker compose -f <file> up should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure if we can not add a test because without this command, it doesn't exit after the tests finish. Example run of docker compose -f docker-compose-test.yaml up on CircleCI. But this is great issue because by that I remembered why did we use docker compose run previously. Command docker compose up doesn't return exit code from container execution, which results in that tests could fail and CI still would be green example run with failing test with docker compose up test. But this means I have to return most of the changes in docker-compose configuration 😞 .

Comment on lines +12 to +22
iex> %{
...> "notification" => <<18, 76, 10, 36, 102, 98, 102, 52, 49, 57, 48, 99, 45, 53, 99, 55, 54, 45, 52,
...> 49, 53, 99, 45, 56, 57, 51, 57, 45, 53, 50, 99, 54, 101, 100, 50, 48, 56, 54,
...> 56, 98, 18, 36, 99, 55, 50, 51, 54, 53, 56, 55, 45, 53, 100, 102, 56, 45, 52,
...> 98, 52, 49, 45, 98, 54, 101, 52, 45, 50, 54, 56, 101, 55, 49, 49, 51, 51, 101,
...> 101, 50>>
...> } |> Jellyfish.WebhookNotifier.receive()
%Jellyfish.Notification.PeerConnected{
room_id: "fbf4190c-5c76-415c-8939-52c6ed20868b",
peer_id: "c7236587-5df8-4b41-b6e4-268e71133ee2"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@mickel8 mickel8 changed the title RTC 386 [RTC-386] Add webhooks Oct 24, 2023
@Rados13 Rados13 merged commit cee29fb into master Oct 26, 2023
@Rados13 Rados13 deleted the RTC-386 branch October 26, 2023 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants