-
Notifications
You must be signed in to change notification settings - Fork 135
Conversation
The
|
Yeah, it's currently hard to find out the reason. In my experience, this usually means the pod of rudr can't be running. Can you try to build the image , deploy and fix it locally. |
To be clear, this is just a drive-by PR. |
I tried your PR locally, but it fails on compiling:
|
@wonderflow It requires rustc 1.41(current stable version). |
Thanks @messense, I have successfully build and it crashes as the following errors:
|
@wonderflow Great, I'll look into it later. |
You could rebase the master branch and the CI will give more detail logs |
@wonderflow Still can't spot the problem in the detailed logs. |
Thanks so much for this great PR, but it's better if you could setup test environment. I'm not very sure that will it possible that the |
|
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.
Thanks so much for your effort and it's great to see such big contribution.
But as you can see, this PR is really complex. I don't know if these changes are all for upgrade dependency.
Hope you could give more information about this PR and help us understand better!
Thanks again!
uses: actions-rs/toolchain@v1 | ||
with: | ||
toolchain: stable | ||
override: true |
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.
Do you mean this is a recommend way to always use latest rust version? Will we have risk to do so?
Dockerfile
Outdated
@@ -1,4 +1,4 @@ | |||
ARG BUILDER_IMAGE=rust:1.38 | |||
ARG BUILDER_IMAGE=rust:1.41 |
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.
please go through this repo and make sure all versions are updated to 1.41, thanks!
@@ -70,11 +66,12 @@ fn main() -> Result<(), Error> { | |||
let client = APIClient::new(cfg_watch); | |||
let mut cnt = 0; | |||
loop { | |||
let req = healthscope_resource.list(&ListParams::default())?; | |||
match client.request::<ObjectList<HealthScopeObject>>(req) { | |||
let req = healthscope_resource.list(&ListParams::default()).unwrap(); |
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.
What will happen if it's None and causes panic?
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'll let you review the over all approach before addressing these small nits.
} | ||
}); | ||
|
||
let server = std::thread::spawn(move || { | ||
let server1 = tokio::spawn(async move { |
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.
maybe we should rename server1
to main_server
}); | ||
|
||
std::thread::spawn(move || { | ||
let server2 = tokio::spawn(async move { |
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.
rename server2
to health_server
will be better
@@ -45,38 +45,38 @@ impl OAMScope { | |||
} | |||
} | |||
/// create will create a real scope instance | |||
pub fn create(&self, owner: meta::OwnerReference) -> Result<(), Error> { | |||
pub async fn create(&self, owner: meta::OwnerReference) -> Result<(), Error> { |
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.
could you give some explanation that why we must change every function(including add\modify\delete\remove\status) to async?
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.
kube-rs
needs async
/await
, and async is invasive, which means that async fns needs to be called in async fns.
To upgrade See https://github.com/dtolnay/async-trait for |
cargo build
&cargo test
passes on my local macOS.Fixes #529