Skip to content

Commit

Permalink
ci: remove smtp port options
Browse files Browse the repository at this point in the history
  • Loading branch information
@IgnisDa
IgnisDa committed Jul 31, 2024
1 parent 15ea505 commit c453dcb
Show file tree
Hide file tree
Showing 7 changed files with 29 additions and 13 deletions.
25 changes: 25 additions & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ sea-orm = { version = "=1.0.0-rc.5", features = [
"debug-print",
"postgres-array",
"macros",
"runtime-tokio-rustls",
"runtime-tokio-native-tls",
"sqlx-postgres",
"with-chrono",
"with-json",
Expand Down
5 changes: 1 addition & 4 deletions apps/backend/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,7 @@ isolang = { version = "=2.4.0", features = ["list_languages"] }
itertools = "=0.13.0"
jsonwebtoken = { version = "=9.3.0", default-features = false }
kinded = "=0.3.0"
lettre = { version = "=0.11.7", features = [
"smtp-transport",
"builder",
], default-features = false }
lettre = "=0.11.7"
markdown = "=1.0.0-alpha.18"
mime_guess = "=2.0.5"
nanoid = { workspace = true }
Expand Down
3 changes: 1 addition & 2 deletions apps/backend/src/notification.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,8 @@ impl NotificationPlatformSpecifics {
config.server.smtp.password.to_owned(),
);

let mailer = SmtpTransport::starttls_relay(&config.server.smtp.server)
let mailer = SmtpTransport::relay(&config.server.smtp.server)
.unwrap()
.port(config.server.smtp.port)
.credentials(credentials)
.build();

Expand Down
3 changes: 0 additions & 3 deletions docs/includes/backend-config-schema.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,9 +206,6 @@ server:
# @envvar SERVER_SMTP_PASSWORD
password: ""

# @envvar SERVER_SMTP_PORT
port: 587

# @envvar SERVER_SMTP_SERVER
server: ""

Expand Down
2 changes: 0 additions & 2 deletions libs/config/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,8 +338,6 @@ pub struct SchedulerConfig {
#[config(rename_all = "snake_case", env_prefix = "SERVER_SMTP_")]
pub struct SmtpConfig {
pub server: String,
#[setting(default = 587)]
pub port: u16,
pub user: String,
pub password: String,
#[setting(default = "Ryot <[email protected]>")]
Expand Down
2 changes: 1 addition & 1 deletion rust-toolchain.toml
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
[toolchain]
profile = "default"
channel = "1.77.2"
channel = "1.80.0"

12 comments on commit c453dcb

@IgnisDa
Copy link
Owner Author

@IgnisDa IgnisDa commented on c453dcb Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @vnghia! I know this is out of the blue but could you help me perhaps? I have not been able to get builds working because of some dependency missing but now that I have added it to my dockerfile, it still does not work. Funny thing is, it works locally but not on github actions. Probably because of platform=amd64.

I would be grateful if you could take a look.

@IgnisDa
Copy link
Owner Author

Choose a reason for hiding this comment

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

Also relevant commit: 15ea505

@vnghia
Copy link
Contributor

@vnghia vnghia commented on c453dcb Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I will take a look at it.

Edit: I see why. Will try to send you a PR soon.

@vnghia
Copy link
Contributor

@vnghia vnghia commented on c453dcb Jul 31, 2024

Choose a reason for hiding this comment

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

@IgnisDa, is there any reason why you want to use openssl instead of rustls ?

@vnghia
Copy link
Contributor

@vnghia vnghia commented on c453dcb Jul 31, 2024

Choose a reason for hiding this comment

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

The fix will be something like this: https://github.com/cross-rs/cross/wiki/FAQ#procedural-macros. The reason is because we are cross compiling, the libssl-dev you are installing on arm64 is still amd64.

@vnghia
Copy link
Contributor

@vnghia vnghia commented on c453dcb Jul 31, 2024

Choose a reason for hiding this comment

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

@IgnisDa not sure where to send the PR so essentially, the fix is jsut

RUN dpkg --add-architecture $TARGETPLATFORM
RUN apt-get update && apt-get install --assume-yes libssl-dev:amd64 libssl-dev:$TARGETPLATFORM

On amd64, TARGETPLATFORM will be amd64 and wont have any effect. On arm64, it will install both libssl-dev:amd64 and libssl-dev:arm64

@IgnisDa
Copy link
Owner Author

Choose a reason for hiding this comment

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

I was able to solve it by removing all the target platform stuff. The reason those were added were because ryot needed to suport sqlite. But since that is no longer a requirement, I got rid of it. Compile times went down by 50%.

One thing I still need help with is testing the amd64 version. I don't own a system of that architecture.

@IgnisDa
Copy link
Owner Author

Choose a reason for hiding this comment

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

The reason I switched to openssl is because some dependencies (eg: radarr-api-rs) do not support feature flags that will allow me to enable the rusttls version. Also the reason I used rusttls back in the day was because the final docker image stage was FROM scratch. Now ryot uses a FROM node:slim so there's no openssl vendoring issues.

@vnghia
Copy link
Contributor

@vnghia vnghia commented on c453dcb Jul 31, 2024

Choose a reason for hiding this comment

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

amd64 is x86_64 so I think you already have it. The problematic one is arm64. I will test this commit for you.

@vnghia
Copy link
Contributor

@vnghia vnghia commented on c453dcb Jul 31, 2024

Choose a reason for hiding this comment

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

@IgnisDa can you add a branch to this commit so I can clone it

@IgnisDa
Copy link
Owner Author

@IgnisDa IgnisDa commented on c453dcb Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@vnghia You can just go ahead with the latest commit of the main branch. I squashed all changes into one commit and force pushed it.

@IgnisDa
Copy link
Owner Author

Choose a reason for hiding this comment

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

Also since builds for both platforms succeeded, you can just download the latest image from ghcr and test it on arm64 directly.

Please sign in to comment.