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

Add flitering of versions. #18

Merged
merged 13 commits into from Nov 1, 2022
Merged

Add flitering of versions. #18

merged 13 commits into from Nov 1, 2022

Conversation

Darth-Ness
Copy link
Contributor

Unfortunately, I wasn't able to confirm it works. It does at least compile without errors. Let me know if you want me to change anything.

@tbrand
Copy link
Contributor

tbrand commented Oct 28, 2022

As I wrote in #13, you have to define --min-version arg frst.
Also, you shouldn't filtering the nodes by while (here) but you should do it in the iterator here

@Darth-Ness
Copy link
Contributor Author

As I wrote in #13, you have to define --min-version arg frst. Also, you shouldn't filtering the nodes by while (here) but you should do it in the iterator here

Okay, it now uses the filter method instead of a while loop.

@tbrand
Copy link
Contributor

tbrand commented Oct 28, 2022

Cool!

  • Avoid parsing args again here but pass it as arg.
  • Is this correct? It seems the opposite.

Please create a latest docker-compose environment to confirm the implementation! Only you have to do is download the latest docker-compose binary here.

@Darth-Ness
Copy link
Contributor Author

Do you mean the operator I used? If so I think it correct.

@Darth-Ness
Copy link
Contributor Author

sudo ./docker-compose-linux-armv7 build outputs

ERROR [negy-negy-node1 internal] load metadata for docker.io/library/debian:buster-slim

@tbrand
Copy link
Contributor

tbrand commented Oct 28, 2022

https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.filter
true elements will be remaining. so it should be n.version >= min_version

negy-gateway/src/main.rs Outdated Show resolved Hide resolved
@@ -55,6 +58,14 @@ async fn fetch_nodes_unselected(node_pool_endpoint: &str) -> Result<Vec<NodeUnse
name: n.name,
version: n.version,
})
.filter(|n| {
if let Some(min_version) = &args.min_version {
n.version.parse::<i32>().unwrap() >= min_version.parse::<i32>().unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't parse as i32 but add semver crate and parse as Version. https://docs.rs/semver/latest/semver/

negy-gateway/src/main.rs Outdated Show resolved Hide resolved
@Darth-Ness
Copy link
Contributor Author

Should I focus on resolving the conflict now?

@tbrand
Copy link
Contributor

tbrand commented Oct 31, 2022

Sure!

@Darth-Ness
Copy link
Contributor Author

Okay, I got the conflict resolved.

@tbrand
Copy link
Contributor

tbrand commented Oct 31, 2022

Nice! The last thing you have to do it add semver create as I wrote.

negy-gateway/src/main.rs Outdated Show resolved Hide resolved
negy-gateway/src/main.rs Outdated Show resolved Hide resolved
negy-gateway/src/main.rs Outdated Show resolved Hide resolved
@Darth-Ness
Copy link
Contributor Author

Okay empty lines removed.

@tbrand
Copy link
Contributor

tbrand commented Oct 31, 2022

Thxs. Please add semver crate as I mentioned.

@Darth-Ness
Copy link
Contributor Author

All right, it now uses the semver crate.

) -> Result<()> {

Copy link
Contributor

Choose a reason for hiding this comment

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

empty line added

@@ -124,11 +135,13 @@ async fn main() -> Result<()> {

let listener = TcpListener::bind(bind_addr).await?;


Copy link
Contributor

Choose a reason for hiding this comment

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

empty line added

@@ -24,6 +25,8 @@ struct Args {
hops: usize,
#[clap(short, long, value_parser)]
auth_token: Option<String>,
min_version: Option<String>,

Copy link
Contributor

Choose a reason for hiding this comment

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

empty line added

@tbrand
Copy link
Contributor

tbrand commented Nov 1, 2022

Looks good!! After you removing the empty lines, I gonna merge this!

@Darth-Ness
Copy link
Contributor Author

Hang on, I forgot to update the cargo files, let me do that before you merge.

@Darth-Ness
Copy link
Contributor Author

Okay got those updated, and empty lines removed.

@tbrand
Copy link
Contributor

tbrand commented Nov 1, 2022

Yup. thanks

@tbrand
Copy link
Contributor

tbrand commented Nov 1, 2022

I think you need to add semver both of /Cargo.toml and /negy-gateway/Cargo.toml. Please confirm your code could be compiled and run successfully. Also, please do not commit any changes which is meaningless in this pull request. Like adding empty lines and removing existing codes. Thanks.

@Darth-Ness
Copy link
Contributor Author

I did fix that one file. Not sure why that happened. And yes, cargo run in the negy-gateway directory work just fine

@tbrand
Copy link
Contributor

tbrand commented Nov 1, 2022

Hey @Darth-Ness, I would like to finish this PR for the next release so I created a new PR to fix this. #22
All of your commits are in it. If it's ok, I would like to merge #22. How does it sound?

@tbrand tbrand mentioned this pull request Nov 1, 2022
@Darth-Ness
Copy link
Contributor Author

Darth-Ness commented Nov 1, 2022

Hey @Darth-Ness, I would like to finish this PR for the next release so I created a new PR to fix this. #22 All of your commits are in it. If it's ok, I would like to merge #22. How does it sound?

That's good, go ahead and merge it whenever your ready. Do you want me to close this?
By the way, when some one makes a PR to one of your repos, you can directly commit to the branch the new commits are coming from (Unless that is turned off)

@tbrand tbrand merged commit 6623c08 into negyio:dev Nov 1, 2022
@tbrand
Copy link
Contributor

tbrand commented Nov 1, 2022

I merged this! Thanks for the contribution!! 🎉 Your name will be listed.

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

2 participants