Refactor k8s rule type annotation parsing/retrieval. #1151

timoreimann · 2017-02-12T00:32:37Z

Carved out of #1129.

Move annotation logic into function.
Constantify strings.
Refactor TestRuleType.
Add test for GetRuleTypeFromAnnotations.

timoreimann · 2017-02-12T00:35:32Z

@emilevauge @errm This is the referenced, original PR trimmed done to the refactoring parts as announced.

Could you please take a look? Thanks!

timoreimann · 2017-02-12T06:11:24Z

provider/kubernetes_test.go

+	tests := []struct {
+		desc             string
+		ingressRuleType  string
+		serviceRuleType  string


Just realized I forgot to remove this one. Will do so later.

timoreimann · 2017-02-22T05:29:40Z

@emile @SantoDE @errm @russ @dtomcej @vdemeester would be grateful for a review, thanks. :-)

vdemeester

LGTM 🐸

dtomcej · 2017-02-22T15:46:45Z

provider/kubernetes.go

-						ruleType = "PathPrefix"
+				if len(pa.Path) > 0 {
+					ruleType, unknown := getRuleTypeFromAnnotation(i.Annotations)
+					switch {


This may be a dumb question, but If you are not switching on a variable, should you still have a default in the event that neither of the cases are matched?

Not a dumb question at all!

I agree with you that there should usually be a default case when you use a switch statement in an exhaustive way: Handle all acceptable inputs and do something for the unexpected case (like returning an error).

The other type of switch usage in Go is a more readable version of multiple / nested / complex if-else statements. This one doesn't necessarily need a default case in my opinion, and it's the one I had in mind here.

Admittedly, I could have also written

if unknown { log.Warnf("Unknown RuleType ...") } if ruleType == "" { ruleType = ruleTypePathPrefix }

I prefer the switch variant because I can use fallthrough to shortcut right to setting a default value in case of an unknown rule type (fallthrough ignores the condition of the following case expression) and don't need to depend onruleType having a particular value (empty string here).

If you think that two if statements would be better, I'm happy to change.

dtomcej

LGTM

timoreimann · 2017-02-23T08:57:43Z

@vdemeester @dtomcej okay to merge?

emilevauge · 2017-02-23T09:45:46Z

@timoreimann I'm still not sure maintaining another rule (incomplete) list is a good idea. Rules are defined https://github.com/containous/traefik/blob/master/rules.go#L107 and again https://github.com/containous/traefik/pull/1151/files#diff-f9e99fb6bddb36d1640e40bc4bd36bc1R22 in kubernetes.go. It duplicates parsing code.
I'm sorry but I'm still considering that this solution would be a lot simpler to maintain.
@errm @dtomcej WDYT ?

timoreimann · 2017-02-23T10:53:50Z

@emilevauge I agree with you in general. However, the PR did not introduce any new rules: It's a refactoring (and test-extending) change only which IMHO already provides an incremental improvement over the previous state. Put in a different way, it still contains the same level of duplication but makes it more readable (agile, yay :-) ).

My suggestion would be to move forward with this PR and follow-up with some de-duplication effort afterwards. That way, we don't conflate a fairly limited and simple refactoring with a bigger change that could possibly be more difficult to design and implement.

Russell-IO

LGTM 👍

timoreimann · 2017-03-01T05:45:01Z

@emile @errm any further thoughts on this one?

- Move annotation logic into function. - Constantify strings. - Refactor TestRuleType. - Add test for GetRuleTypeFromAnnotations.

emilevauge · 2017-03-03T15:10:22Z

However, the PR did not introduce any new rules: It's a refactoring (and test-extending) change only which IMHO already provides an incremental improvement over the previous state. Put in a different way, it still contains the same level of duplication but makes it more readable (agile, yay :-) ).

You're right, in my opinion, we shouldn't have accepted the previous state.
Let's move on. We will really need to de-duplicate all these rules soon or later.

emilevauge

LGTM!

timoreimann force-pushed the refactor-k8s-rule-type-annotation-logic branch from dd29713 to 69315de Compare February 12, 2017 00:33

timoreimann commented Feb 12, 2017

View reviewed changes

timoreimann force-pushed the refactor-k8s-rule-type-annotation-logic branch from 69315de to 0d1eb9a Compare February 12, 2017 21:38

timoreimann force-pushed the refactor-k8s-rule-type-annotation-logic branch 3 times, most recently from d36f4ce to 1f5c33c Compare February 21, 2017 23:18

vdemeester added the status/2-needs-review label Feb 22, 2017

vdemeester requested review from errm, Russell-IO, emilevauge and dtomcej February 22, 2017 09:54

vdemeester approved these changes Feb 22, 2017

View reviewed changes

dtomcej reviewed Feb 22, 2017

View reviewed changes

timoreimann force-pushed the refactor-k8s-rule-type-annotation-logic branch from 1f5c33c to 12d7904 Compare February 22, 2017 18:17

dtomcej approved these changes Feb 22, 2017

View reviewed changes

Russell-IO approved these changes Feb 23, 2017

View reviewed changes

timoreimann force-pushed the refactor-k8s-rule-type-annotation-logic branch from 12d7904 to f68e016 Compare March 1, 2017 13:48

Refactor k8s rule type annotation parsing/retrieval.

f3598e6

- Move annotation logic into function. - Constantify strings. - Refactor TestRuleType. - Add test for GetRuleTypeFromAnnotations.

timoreimann force-pushed the refactor-k8s-rule-type-annotation-logic branch from f68e016 to f3598e6 Compare March 3, 2017 12:33

emilevauge approved these changes Mar 3, 2017

View reviewed changes

emilevauge added status/3-needs-merge and removed status/2-needs-review labels Mar 3, 2017

timoreimann merged commit 38c0cf7 into traefik:master Mar 3, 2017

timoreimann deleted the refactor-k8s-rule-type-annotation-logic branch March 3, 2017 17:49

ldez added the area/provider/k8s/ingress label May 2, 2017

ldez added this to the 1.3 milestone May 19, 2017

ldez added kind/enhancement a new or improved feature. and removed status/3-needs-merge labels May 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor k8s rule type annotation parsing/retrieval. #1151

Refactor k8s rule type annotation parsing/retrieval. #1151

timoreimann commented Feb 12, 2017

timoreimann commented Feb 12, 2017

timoreimann Feb 12, 2017

timoreimann Feb 13, 2017

timoreimann commented Feb 22, 2017

vdemeester left a comment

dtomcej Feb 22, 2017

timoreimann Feb 22, 2017

dtomcej Feb 22, 2017

dtomcej left a comment

timoreimann commented Feb 23, 2017

emilevauge commented Feb 23, 2017 •

edited

Loading

timoreimann commented Feb 23, 2017 •

edited

Loading

Russell-IO left a comment

timoreimann commented Mar 1, 2017

emilevauge commented Mar 3, 2017

emilevauge left a comment

Refactor k8s rule type annotation parsing/retrieval. #1151

Refactor k8s rule type annotation parsing/retrieval. #1151

Conversation

timoreimann commented Feb 12, 2017

timoreimann commented Feb 12, 2017

timoreimann Feb 12, 2017

Choose a reason for hiding this comment

timoreimann Feb 13, 2017

Choose a reason for hiding this comment

timoreimann commented Feb 22, 2017

vdemeester left a comment

Choose a reason for hiding this comment

dtomcej Feb 22, 2017

Choose a reason for hiding this comment

timoreimann Feb 22, 2017

Choose a reason for hiding this comment

dtomcej Feb 22, 2017

Choose a reason for hiding this comment

dtomcej left a comment

Choose a reason for hiding this comment

timoreimann commented Feb 23, 2017

emilevauge commented Feb 23, 2017 • edited Loading

timoreimann commented Feb 23, 2017 • edited Loading

Russell-IO left a comment

Choose a reason for hiding this comment

timoreimann commented Mar 1, 2017

emilevauge commented Mar 3, 2017

emilevauge left a comment

Choose a reason for hiding this comment

emilevauge commented Feb 23, 2017 •

edited

Loading

timoreimann commented Feb 23, 2017 •

edited

Loading