-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
chore: Use Kotlin for ReVanced API #169
base: main
Are you sure you want to change the base?
Conversation
Support and features are lacking
What is the motivation to rewrite it in kotlin? |
Personally, it makes it easier for me to maintain and reduces the amount of languages as we already use Kotlin. With our current maintainer being short on time, especially in the future, the Kotlin version will be easier to look into for me or other Kotlin devs we have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. I have some suggestions related to the infrastructure side of things that are put in their respective files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both this and the devcontainer.json
files are quite important as they allow easy deployment of a development environment without the need to manually set up all the requirements for the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is support for codeql on Kotlin as well, might want to bring this back at least for PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I highly recommend using either the official eclipse-temurin
images or the ibm-semeru-runtimes
(if planning on using OpenJ9 instead of HotSpot). The Zulu images lag security updates one or two days most of the time and are larger (almost double the size iirc).
Another mistake that also existed in the version currently deployed of our API is to not force the application to run as a non-root user inside of the container, which is a security risk. Please refer to: https://redhatgov.io/workshops/security_containers/exercise1.2/ and just make sure that whatever user/group created has an id between 100 and 499 as per Linux Standard Base Core Specification (Linux Standard Base Core Specification).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want the docker container to stay up to date with the base image in case security issues pop up between application releases. It is possible to use something like https://github.com/marketplace/actions/docker-image-update-checker or to at least update the images every week or so (so we do not risk releases that are months apart also meaning we take months to update a vulnerable runtime).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependabot also supports Kotlin and keeping dependencies up to date here is very important, in case something like ktor have a huge vuln like log4j we want the dependency checker to keep us informed so we can quickly integrate the PR and release a new version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to change the license of a hosted service from AGPL to GPL. The idea of the AGPL license is that the authors of derivative code must release whatever changes they have made to the code even if the only way to access the service is via the web or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This a breaking API change to be served from the same domain as the earlier, and as such API versioning should follow existing versions. This would be version 3 of the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using commonly used ports like 8080 to protect deployment from common pitfalls like conflicting with existing software. 8080 is an official alternative HTTP port (80, 8080 and 8008), just pick something that is not listed here: https://www.wikiwand.com/en/List_of_TCP_and_UDP_port_numbers.
Also, make sure to set the UUID and GUID for the user that should run the application inside of the container to be identical to the one set in the Dockerfile
.
About
This PR rewrites this project into Kotlin. Ktor is used for the API.
Todo before merge
Review