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

adds working code and test for altering a package name to make it val… #6524

Merged
merged 3 commits into from May 9, 2019

Conversation

Projects
None yet
6 participants
@nellshamrell
Copy link
Contributor

commented May 7, 2019

…id as a Kubernetes resource name

Fixes #5827

Signed-off-by: Nell Shamrell nellshamrell@gmail.com

@chef-expeditor

This comment has been minimized.

Copy link

commented May 7, 2019

Hello nellshamrell! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

adds working code and test for altering a package name to make it val…
…id as a Kubernetes resource name

Signed-off-by: Nell Shamrell <nellshamrell@gmail.com>

Signed-off-by: Nell Shamrell <nellshamrell@gmail.com>

@nellshamrell nellshamrell force-pushed the nell-fix-exporters branch from 7bf45a7 to 054d0e1 May 7, 2019

@@ -151,13 +154,20 @@ impl Manifest {
environment })
}

/// Generates the manifest as a string and writes it to `write`.
/// Generates manifest as a string and writes it to `write`.

This comment has been minimized.

Copy link
@nellshamrell

nellshamrell May 7, 2019

Author Contributor

Minor bit that happened when moving things around - please ignore this (I don't think it's worth another commit to correct it back to what it was, the meaning and intention is still clear)

fn formatted_metadata_name(metadata_name: &str) -> String {
let re = Regex::new(r"\.").unwrap();
re.replace_all(metadata_name, "-").to_string()
}

This comment has been minimized.

Copy link
@raskchanky

raskchanky May 8, 2019

Member

Using a Regex to replace periods with hyphens feels a little heavyweight, in my opinion. You could accomplish the same thing with name.replace(".", "-").

This comment has been minimized.

Copy link
@nellshamrell

nellshamrell May 8, 2019

Author Contributor

That's an excellent point, making that change now.

This comment has been minimized.

Copy link
@nellshamrell

nellshamrell May 8, 2019

Author Contributor

Done!

replaces regex with simpler string replace method
Signed-off-by: Nell Shamrell <nellshamrell@gmail.com>
@@ -101,6 +101,8 @@ impl Manifest {
.map(str::to_string)
.unwrap_or_else(|| format!("{}-{}", pkg_ident.name, version_suffix));

let formatted_resource_name = Manifest::formatted_metadata_name(&name);

This comment has been minimized.

Copy link
@christophermaier

christophermaier May 8, 2019

Contributor

Should this variable be called formatted_metadata_name, or should the function be called Manifest::formatted_resource_name?

(Just curious what, if any, the distinction is)

This comment has been minimized.

Copy link
@nellshamrell

nellshamrell May 8, 2019

Author Contributor

There's not, I'm happy to go either way.

This comment has been minimized.

Copy link
@christophermaier

christophermaier May 8, 2019

Contributor

I'd say we should use whichever one is more natural / appropriate in a Kubernetes context (if indeed one actually is more appropriate than the other).

Then have the variable named similarly to the function(any time there's a difference, I always wonder if it's significant 😄 )

This comment has been minimized.

Copy link
@nellshamrell

nellshamrell May 9, 2019

Author Contributor

Fixing this now!

This comment has been minimized.

Copy link
@nellshamrell

nellshamrell May 9, 2019

Author Contributor

Fixed!

@@ -101,6 +101,8 @@ impl Manifest {
.map(str::to_string)
.unwrap_or_else(|| format!("{}-{}", pkg_ident.name, version_suffix));

let formatted_resource_name = Manifest::formatted_metadata_name(&name);

This comment has been minimized.

Copy link
@raskchanky

raskchanky May 8, 2019

Member

I should've mentioned this in my initial review, but given that the replace is just a single line of code, you could do it here, and eliminate the separate function. It would simplify things a bit.

This comment has been minimized.

Copy link
@nellshamrell

nellshamrell May 8, 2019

Author Contributor

Good point, but I preferred to put formatted_metadata_name in a separate method so that I could test it. The "new_from_cli_matches" method which calls this method is extremely difficult to test (it currently has no test coverage on it - the existing tests cover the "generate" method, but not the "new_from _cli_matches" method). This exporter does need a much larger refactor to make it easier to test and to work in, but I chose the simpler, easier to test solution for now.

This comment has been minimized.

Copy link
@nellshamrell

nellshamrell May 8, 2019

Author Contributor

In the future, I would certainly be open to doing a larger scale refactor, but with the list of priorities right now the simpler, slightly better solution seemed to be the best option.

This comment has been minimized.

Copy link
@christophermaier

christophermaier May 8, 2019

Contributor

👍 for better testability!


// Replaces any periods in the metadata name with hyphens
// To make it a valid Kubernetes resource name
fn formatted_metadata_name(metadata_name: &str) -> String {

This comment has been minimized.

Copy link
@elliott-davis

elliott-davis May 8, 2019

Contributor

Since this function is really just wrapping the replace function I think it and the accompanying test can be removed

This comment has been minimized.

Copy link
@nellshamrell

nellshamrell May 8, 2019

Author Contributor

For thoroughness sake (and to provide a good examples for future refactoring) I would much rather leave it as a separate method that can easily be tested.

The "new_from_cli_matches" method which calls this method is extremely difficult to test (it currently has no test coverage on it - the existing tests cover the "generate" method, but not the "new_from _cli_matches" method). This exporter does need a much larger refactor to make it easier to test and to work in, but I chose the simpler, easier to test solution for now.

This comment has been minimized.

Copy link
@christophermaier

christophermaier May 8, 2019

Contributor

I disagree... this is surfacing the heretofore unhandled and unacknowledged fact that we need to be generating names in a particular way for them to be valid. Breaking it out as a separate function with tests is an important first step in the future refactoring that @nellshamrell mentioned.

This comment has been minimized.

Copy link
@baumanj

baumanj May 8, 2019

Member

I agree with the separate function for Manifest::formatted_metadata_name since it conveys why we're doing this rather than just what we're doing as inlining the replace call would.

As far as the test goes, I think we can do without it for now since it's simple enough to verify by inspection that I don't think it's worth the extra code/CI time. But if this function were to become more complex, I'd say that's an ideal time to add a test which serves to better document it and avoid regression. That's another benefit of factoring this out into it's own function; it's less work to add the test later and therefore more likely to happen.

This comment has been minimized.

Copy link
@nellshamrell

nellshamrell May 8, 2019

Author Contributor

Hey @baumanj - thank you for the feedback. I would prefer to leave the test in, personally, to avoid potentially regressions as the giant "new_from_cli_matches" method is refactored in the future - particularly as the formatting is crucial to produce a valid Kubernetes config. Ideally, formatted_metadata_name would be tested through an integration test on the larger "new_from_cli_matches" method. When that integration test is eventually added, we will no longer have need for this unit test. While we are in this middle ground - particularly as this is a tricky bit of the code base and needs to be so specific to be valid for Kubernetes - I would prefer to leave the unit test in. I feel the code time has already been spent and the CI time is minimal at this point.

This comment has been minimized.

Copy link
@baumanj

baumanj May 8, 2019

Member

That sounds good. We can have differences of opinion, and it's not worth spending too much energy on this particular point. I like when we all feel comfortable sharing our thoughts constructively and then defer to the author's prerogative so that we can keep things moving. When I feel strongly about something as a reviewer, I try to say why and find a common understanding. The existence of this one test isn't something I feel strongly about, but I appreciate you hearing my feedback.

@nellshamrell

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Since this has come up in a couple of comments on the code, I thought I would make a general comment here.

I chose to make a very small, easily testable function for formatting the resource name as a deliberate design decision.

I chose not to add it into the larger "new_from_cli_matches" intentionally. The "new_from_cli_matches" method has no current test coverage (the existing tests are all for the "generate" method). The "new_from_cli_matches" method needs a major refactor to make it straightforward to test. I've chosen not to take on that refactor at this time due to the amount of priorities we already have on our plate.

What I did choose to do was to start laying out the groundwork for a larger refactor and move us to more testable, modularized code by creating a new method that could easily be tested.

@gpeers

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Since this has come up in a couple of comments on the code, I thought I would make a general comment here.

I chose to make a very small, easily testable function for formatting the resource name as a deliberate design decision.

I chose not to add it into the larger "new_from_cli_matches" intentionally. The "new_from_cli_matches" method has no current test coverage (the existing tests are all for the "generate" method). The "new_from_cli_matches" method needs a major refactor to make it straightforward to test. I've chosen not to take on that refactor at this time due to the amount of priorities we already have on our plate.

What I did choose to do was to start laying out the groundwork for a larger refactor and move us to more testable, modularized code by creating a new method that could easily be tested.
hey @nellshamrell, i like the thought you've put into this. thanks for pushing us in the testability direction. 😄

makes method name clearer
Signed-off-by: Nell Shamrell <nellshamrell@gmail.com>
@christophermaier
Copy link
Contributor

left a comment

tenor-256628168

@nellshamrell nellshamrell merged commit e2647a3 into master May 9, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #1752 passed and blocked (14 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #1636 passed and blocked (13 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

chef-ci added a commit that referenced this pull request May 9, 2019

Update CHANGELOG.md with details from pull request #6524
Obvious fix; these changes are the result of automation not creative thinking.

@raskchanky raskchanky deleted the nell-fix-exporters branch May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.