-
Notifications
You must be signed in to change notification settings - Fork 30
feat: switch Rust startup to Rust only #1255
Conversation
|
||
[dependencies.config] | ||
git = "https://github.com/mehcode/config-rs" | ||
rev = "e8fa9fee96185ddd18ebcef8a925c75459111edb" |
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.
FWIW rev
may not be necessary here as you're also using a Cargo.lock
file which has the exact revision
🎉 |
3a692bb
to
be73bc0
Compare
autopush_rs/src/settings.rs
Outdated
format!( | ||
"{}://{}:{}", | ||
self.endpoint_scheme, | ||
self.endpoint_hostname.as_ref().unwrap_or(&hostname), |
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.
for just the connection node it doesn't make sense to fallback to hostname
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.
It's used to generate the endpoint, it needs some value from somewhere.
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.
Can we just start expecting it here then? The python's handling of hostnames is pretty confusing
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.
The tests assume the endpoint hostname will be set to the hostname. They'd all have to change too.
Dockerfile.python27
Outdated
@@ -14,7 +14,7 @@ RUN \ | |||
make clean && \ | |||
pip install -r requirements.txt && \ | |||
python setup.py develop && \ | |||
cd autopush_rs && \ | |||
cargo install && \ |
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.
needs --path
pub endpoint_scheme: String, | ||
pub endpoint_hostname: Option<String>, | ||
pub endpoint_port: u16, | ||
pub crypto_key: String, |
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 is an append (List[str]) in main_argparse. Can Config do something 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.
Not if we want it to work with env vars.
Codecov Report
@@ Coverage Diff @@
## master #1255 +/- ##
=========================================
+ Coverage 99.99% 100% +<.01%
=========================================
Files 60 60
Lines 10547 10554 +7
=========================================
+ Hits 10546 10554 +8
+ Misses 1 0 -1
Continue to review full report at Codecov.
|
641c490
to
892d438
Compare
Co-authored by: Phil Jenvey <pjenvey@underboss.org> Closes #1243
Co-authored by: Phil Jenvey pjenvey@underboss.org
Closes #1243