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

chore: Use Kotlin for ReVanced API #169

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from
Draft

chore: Use Kotlin for ReVanced API #169

wants to merge 27 commits into from

Conversation

oSumAtrIX
Copy link
Member

@oSumAtrIX oSumAtrIX commented Feb 1, 2024

About

This PR rewrites this project into Kotlin. Ktor is used for the API.

Todo before merge

  • Add issue templates
  • Setup repository tags correctly (proper naming for example)

Review

  • All the routes work, and none are missing
    • Caching is done correctly
    • Headers are set correctly
    • Auth works correctly
    • Cors works correctly
  • Persistence layer is correctly implemented
  • Rate limits are not an issue
  • Dockerfile and Docker Compose are setup correctly
  • CI/CD works properly
    • Version is bumped correctly
    • Pre-releases are built and published correctly, also to the Docker registry
    • Release assets are uploaded, and the changelog is generated correctly
    • Release workflow should probably not run outside of dev or main

@indrastorms
Copy link

What is the motivation to rewrite it in kotlin?

@oSumAtrIX
Copy link
Member Author

oSumAtrIX commented Feb 1, 2024

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

Copy link
Member

@alexandreteles alexandreteles left a 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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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).

Copy link
Member

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).

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

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.

None yet

3 participants