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.

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

Play 3.0 upgrade #1214

Merged
merged 6 commits into from
Apr 25, 2024
Merged

Play 3.0 upgrade #1214

merged 6 commits into from
Apr 25, 2024

Conversation

davidfurey
Copy link
Member

What does this change?

Minimal changes to upgrade to Play 3.0

How to test

How can we measure success?

Have we considered potential risks?

Images

Accessibility

@davidfurey davidfurey requested a review from a team as a code owner April 10, 2024 09:30
@davidfurey
Copy link
Member Author

Tests on Common and API-models haven't been run, and should be before this is merged

Copy link
Contributor

@waisingyiu waisingyiu left a comment

Choose a reason for hiding this comment

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

Thank you @davidfurey for upgrading n10n to Play3.0.

I tried sending a test notification on CODE with this branch. However, the harvester lambda threw this exception and I'm not sure if it is relevant
Screenshot 2024-04-11 at 17 37 28

Copy link
Contributor

@waisingyiu waisingyiu left a comment

Choose a reason for hiding this comment

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

I have tested the latest snapshot and a test notification has been sent successfully.

It appears that we have to exclude com.typesafe.play bundles explicitly when the project depends on "ai.x" %% "play-json-extensions" (because it brings in the old play-json transitively). I am wondering whether we should also add the exclusion rule to the subproject notification, registration and report? Or have I missed anything (as the notification and registration API can work properly on CODE)?

@davidfurey
Copy link
Member Author

I have tested the latest snapshot and a test notification has been sent successfully.

It appears that we have to exclude com.typesafe.play bundles explicitly when the project depends on "ai.x" %% "play-json-extensions" (because it brings in the old play-json transitively). I am wondering whether we should also add the exclusion rule to the subproject notification, registration and report? Or have I missed anything (as the notification and registration API can work properly on CODE)?

You are correct, the exclusion should also be applied to notification, registration and report. I've pushed a commit with that

Copy link
Contributor

@waisingyiu waisingyiu left a comment

Choose a reason for hiding this comment

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

I tested the changes by sending a test notification and my app on Android simulator was able to receive it.

Thank you @davidfurey for upgrading the n10n service to Play 3.0

@davidfurey
Copy link
Member Author

@waisingyiu do you think we have done sufficient testing to merge this?

@waisingyiu
Copy link
Contributor

@waisingyiu do you think we have done sufficient testing to merge this?

Yes, I think the testing should be sufficient. I ran the following tests

  • sent a request to registration API and received a valid response
  • sent a test notification via notification API and it was received on android simulator. The whole notification delivery pipeline should be working
  • sent a request to report API and received a valid response
  • checked several lambddas - eventconsumer, reportextractor, fakebreakingnews, schedule- and they returned success

I am happy to merge it and keep an eye on it tomorrow morning if it helps?

@davidfurey
Copy link
Member Author

I am happy to merge it and keep an eye on it tomorrow morning if it helps?

That would be great, thanks!

@waisingyiu waisingyiu merged commit ae24708 into main Apr 25, 2024
11 checks passed
@waisingyiu waisingyiu deleted the play-3.0-upgrade branch April 25, 2024 09:38
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.

2 participants