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.

Sign up for GitHub

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

Jump to bottom

Support protocol in port mappings #8

Merged
merged 4 commits into from
Oct 16, 2017
Merged

Support protocol in port mappings #8

merged 4 commits into from
Oct 16, 2017

Conversation

camjackson
Copy link
Contributor

@camjackson camjackson commented Oct 16, 2017 •
edited
Loading

Reference: faradayio/cage#75

Still a work in progress:

  • Add new struct and field as described in the linked issue above
  • Implement traits for the new struct
  • Update unit tests
  • Cleanup
  • Integrate with cage locally and verify that it works

@camjackson camjackson mentioned this pull request Oct 16, 2017
Closed
@camjackson camjackson changed the title WIP: Support protocol in port mappings Support protocol in port mappings Oct 16, 2017
@camjackson
Copy link
Contributor Author

This is ready for review now!

I integrated it with cage locally and tested it with the hello example project. AFAICT, it works 😃

As for the build, it's failing because of an unused macro, which I didn't add. On my laptop, if I turn off warnings and run the tests, I get failing tests on the master branch. Is that expected?

failures:
    v2::build_may_be_a_struct
    v2::command_line_may_be_parsed
    v2::command_line_may_be_shell_code
    v2::extends_can_be_roundtripped
    v2::external_volume_can_be_converted_from_and_to_yaml
    v2::file_can_be_converted_from_and_to_yaml_version_2
    v2::file_can_be_converted_from_and_to_yaml_version_2_1
    v2::network_handles_driver_correctly
    v2::network_handles_external_name_correctly
    v2::network_handles_external_true_correctly
    v2::service_handles_sample_fields_correctly
    v2::service_networks_supports_map
    v2::volume_with_driver_can_be_converted_from_and_to_yaml

emk
emk requested changes Oct 16, 2017
View reviewed changes
Copy link
Owner

@emk emk left a comment

Choose a reason for hiding this comment

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

Wow! This is a great patch, and I'm impressed to see it ready so fast.

I have just a couple of minor suggestions to look at before we merge this. If you have a moment to respond to these today, I'll try to get the new cage out today or tomorrow.

Once again, thank you so much for tackling this!

src/v2/port_mapping.in.rs Outdated
/// Transmission Control Protocol, the default.
TCP,
/// User Datagram Protocol.
UDP
Copy link
Owner

@emk emk Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Just an incredibly minor style nit. :-) Rust seems to prefer writing acronyms in APIs as Tcp and Udp most of the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

src/v2/port_mapping.in.rs Outdated
@@ -145,25 +185,40 @@ impl fmt::Display for PortMapping {
if let Some(ports) = self.host_ports {
write!(f, "{}:", ports)?;
}
write!(f, "{}", self.container_ports)
write!(f, "{}/", self.container_ports)?;
write!(f, "{}", self.protocol)
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we want to skip the / and the protocol if self.protocol.is_default()? This might help avoid confusing downstream tools that don't understand UDP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

src/v2/port_mapping.in.rs Outdated
let fields: Vec<_> = ports_and_protocol.split("/").collect();
match fields.len() {
1 => Ok((fields[0], Protocol::TCP)),
2 => Ok((fields[0], FromStr::from_str(fields[1])?)),
Copy link
Owner

Choose a reason for hiding this comment

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

You can write Protocol::from_str in Rust (using the name of the class, and not just the trait), and it would probably be clearer in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I copied this from a little further down where it does:

//...
                    host_ports: Some(FromStr::from_str(fields[1])?),
                    container_ports: FromStr::from_str(fields[0])?,
//...

src/v2/port_mapping.in.rs Outdated
@@ -195,24 +252,59 @@ impl FromStr for PortMapping {
}

#[test]
fn port_mapping_should_have_a_string_representation() {
fn port_mapping_can_be_formatted_as_a_string() {
Copy link
Owner

Choose a reason for hiding this comment

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

Could we maybe considering keeping the existing test case (the one with the round-trip string conversions in both directions) intact, and:

  1. Add a new 127.0.0.1:80:80/udp test case to the round-trip conversions?
  2. Add a non-round-trip test case just for things like 127.0.0.1:80:80/tcp, which should format as 127.0.0.1:80:80, as per the other suggestions?

I really like the round-trip tests here, because if we maintain two separate lists, somebody will forget to update one direction sooner or later. So the idea is (1) round-trip tests for everything that round-trips, and (2) one-way parsing test cases for syntax we'll never output. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I like that approach, will do that now.

@emk
Copy link
Owner

emk commented Oct 16, 2017

Oh, yeah: The unused macro. You can just remove #[deny(warnings)] in libs.rs.

@camjackson
Only print the protocol if it's non default. Also:
730b3b8 
- Make protocol acronyms Capitalised
- Bring back the round-trip test, and test the non-round
trip cases separately
@camjackson
Copy link
Contributor Author

camjackson commented Oct 16, 2017 •
edited
Loading

Ready for your eyes again @emk! Do feel free to suggest more changes if it needs it.

Full disclosure: I work with @nsriram, so the reason I jumped in and patched this up quickly is because I need it too! 😁

emk
emk approved these changes Oct 16, 2017
View reviewed changes
Copy link
Owner

@emk emk left a comment

Choose a reason for hiding this comment

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

Looks great!

@emk emk merged commit ae4c847 into emk:master Oct 16, 2017
@camjackson camjackson deleted the protocols branch October 16, 2017 12:44
@emk
Copy link
Owner

emk commented Oct 16, 2017

I'm getting a lot of unit test failures after merging this patch. I don't think they caused by these changes—I think one of our dependencies has been upgraded and broke something—but I need to sort this out first.

@camjackson
Copy link
Contributor Author

Yeah I was getting unit test failures on master/HEAD before I started work on this. Let me know if I can help at all.

@emk
Copy link
Owner

emk commented Oct 16, 2017

This should now be released in 0.0.49 on crates.io. Thank you! We can move back to the cage repo now. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emk emk emk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@camjackson @emk