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

Refactor gceurlmap to a struct representation #254

Merged
merged 1 commit into from
May 9, 2018

Conversation

rramkumar1
Copy link
Contributor

Will fix tests once approach looks good.

/assign @nicksardo

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 8, 2018
@rramkumar1 rramkumar1 force-pushed the gceurlmap-refactor branch from 2ca3194 to dd5bec2 Compare May 8, 2018 22:14
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 8, 2018
@rramkumar1 rramkumar1 force-pushed the gceurlmap-refactor branch from dd5bec2 to 7963173 Compare May 8, 2018 23:07
@rramkumar1
Copy link
Contributor Author

rramkumar1 commented May 8, 2018

The tests in controller_test.go need a really big refactor so I am commenting out the portions which cause the unit tests to fail for now. I would like to really dig into fixing how those tests are written in a separate PR.

@rramkumar1 rramkumar1 force-pushed the gceurlmap-refactor branch from 7963173 to fcb69c1 Compare May 9, 2018 19:03
@rramkumar1
Copy link
Contributor Author

Decided to modify tests and fakes accordingly to make all tests work. Still want to dig deeper into controller_test.go later and fix it up.

@rramkumar1 rramkumar1 force-pushed the gceurlmap-refactor branch from fcb69c1 to 2fa7e1a Compare May 9, 2018 19:11
continue
}
pathToBackend := map[string]string{}
// Using a mapping of regex -> backend here ensures that we don't
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use the word regex as this confuses users. Stick with path?


// PathRule encapsulates the information for a single path -> backend mapping.
type PathRule struct {
// Path is a regex.
Copy link
Contributor

Choose a reason for hiding this comment

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

sans regex

@rramkumar1 rramkumar1 force-pushed the gceurlmap-refactor branch from 2fa7e1a to 0734d79 Compare May 9, 2018 21:31
@rramkumar1 rramkumar1 force-pushed the gceurlmap-refactor branch from 0734d79 to 0e26a15 Compare May 9, 2018 21:33
@nicksardo
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2018
@nicksardo nicksardo merged commit 6c035bd into kubernetes:master May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants