Update SF labels to use explicit methods and add missing labels #16

lawrencegripper · 2017-12-15T13:36:16Z

Hi,

This is WIP at the moment but I wanted to check in before I go much further - in case this is the wrong direction.

I've started to add a few additional labels using explicit methods, as discussed in Review supported labels in the provider and add those missing #3. Adding PassTLSCert, FrameDeny and RequestHeaders
I've tried to bring existing SF labels in line with those in providers/labels so isExposed -> Enabled
To make sure they behave as expected I've created a new test
I've renamed existing label functions to ensure those that return a func are have func in the name and those that don't don't

Let me know if this all makes sense and I'll continue to add the remaining labels from the docker provider implementation in v1.5

ldez · 2017-12-15T14:35:35Z

FYI:

master -> Træfik 1.6
v1.0 -> Træfik 1.5

I assume the tests are in WIP but I think we must change the way that we test:

updateConfig: must have tests only on the configuration update (add/remove/change)
buildConfiguration: must have tests on labels

We must remove tests on labels from updateConfig tests.

lawrencegripper · 2017-12-15T14:54:56Z

Thanks. Wasn't sure what to work off :)

Good Idea, I'll reformat the labels tests I added to use buildConfiguration.

Shall I tackle splitting the updateConfig test in this PR or handle it separately? I think it would be better split into three tests

Update config - Is the config sent to the channel. Content is not relevant.
Services present - This can use BuildConfiguration. It should check services exposed by the sfsdk are present in the config message
Labels tests - Use the new test TestFrontendLabelConfig and create TestBackendLabelConfig to cover these cases

Sound like a good plan?

On an unrelated note is there a reason the MakeFile uses this syntax gometalinter --vendor --enable=misspell ./... not gometalinter --vendor --enable=misspell $(PKGS)

ldez · 2017-12-15T15:08:27Z

Sound like a very good plan! 😄

gometalinter support vendor system via --vendor since go1.5/1.6

Before go1.9, go test runs tests from vendor folder 😞 I use $(PKGS) in go test due that.
But this can be remove because Traefik support only go1.9+.

lawrencegripper · 2017-12-15T16:43:41Z

Test refactoring is done (hopefully), if it looks good I'll add in additional use cases and start working on the additional labels too.

lawrencegripper · 2017-12-22T15:30:21Z

@ldez Hey, gometalinter fails on gocyclo tests for the labels test as it's quite long one now.

I've looked at trying to break it up but, imo, it makes it harder to read and update (for example pulling out each validate func into a full func rather than inline). Happy to split it up if that would be your preference.

No rush on this as I'm on leave from today until the new year so unlikely to be doing a huge amount on it.

ldez · 2017-12-22T15:42:50Z

You can split in 2 test methods:

one for frontend labels
one for backend labels

WDYT?

lawrencegripper · 2017-12-22T15:48:35Z

Good call but already have that. gocyclo still fails because there are lots of frontend labels to test, I'll maybe have a play at pulling out the testcase items into loosely grouped vars outside of the test method. Like security, headers etc.

I did stop for a bit and think that the tests were overkill but as it's easy to edit the tmpl file and not notice it causing a bug with a label I do think they are of value.

WDYT?

ldez · 2017-12-22T16:20:02Z

I will try to customize the gocylo configuration.

ldez · 2018-01-05T10:28:30Z

gocyclo doesn't have "real" configuration.

I think we can add // nocyclo on the frond of the test method, not really happy with this but it's the only way.

// nocyclo
func TestFrontendLabelConfig(t *testing.T){
// ...
}

// nocyclo
func TestBackendLabelConfig(t *testing.T){
// ...
}

ldez · 2018-01-05T12:56:45Z

Sorry mistake, it's // nolint: gocyclo

lawrencegripper · 2018-01-05T13:04:45Z

np, should have tested it locally - did the trick there are some failing tests which I'm expecting during the refactoring

lawrencegripper · 2018-01-09T17:40:14Z

@ldez hopefully this is no-longer WIP and ready for review

jjcollinge

@lawrencegripper few quick notes

jjcollinge · 2018-01-22T09:01:08Z

servicefabric_tmpl.go

@@ -58,7 +61,7 @@ const tmpl = `
        {{range $replica := $partition.Replicas}}
          {{if isPrimary $replica}}


How to route to secondaries when listenOnSecondary enabled - can be added later.

Oooh interesting, lets chat about this one maybe best as a separate PR as this is already pretty chunky

jjcollinge · 2018-01-22T09:10:17Z

servicefabric_tmpl.go

    {{range $partition := $service.Partitions}}
-      {{if eq $partition.ServiceKind "Stateless"}}
+      {{if eq $partition.ServiceKind "Stateless"}} 


Should this be a function isStateless rather than a hard-coded string?

Done now, brain dead simple func but added some tests too

jjcollinge · 2018-01-22T09:22:56Z

servicefabric.go

-
-	return p.GetConfiguration(tmpl, sfFuncMap, templateObjects)
-}
-
 func getDefaultEndpoint(instance replicaInstance) string {


nit: order functions in servicefabric.go for readability:

local functions i.e. getClusterServices

template functions i.e. getDefaultEndpoint

helper functions i.e. decodeEndpointData

Good spot, I've gone a step further and moved template funcs into config.go so only funcs related to enumerating services are left in this file. Think it makes things cleaner - what do you think?

Yeah makes sense.

ldez

LGTM

… with labels file.

…l name not suffix

jjcollinge · 2018-03-22T08:46:50Z

LGTM

lawrencegripper requested a review from ldez December 15, 2017 13:36

lawrencegripper force-pushed the addlabels branch from f8b299d to e867263 Compare December 15, 2017 13:51

ldez changed the base branch from mrg-current-1.0 to master December 15, 2017 14:16

ldez added WIP kind/enhancement labels Dec 15, 2017

ldez added this to the traefik-v1.6 milestone Dec 15, 2017

lawrencegripper force-pushed the addlabels branch from 6ae31ea to 1799674 Compare December 15, 2017 16:35

lawrencegripper force-pushed the addlabels branch from 1799674 to f0ba5e2 Compare December 15, 2017 16:48

lawrencegripper force-pushed the addlabels branch from 77654ad to 438aafc Compare January 5, 2018 13:03

lawrencegripper force-pushed the addlabels branch from 5b00db2 to 4a26c56 Compare January 9, 2018 11:57

ldez added status/2-needs-review and removed WIP labels Jan 9, 2018

jjcollinge reviewed Jan 22, 2018

View reviewed changes

lawrencegripper mentioned this pull request Mar 19, 2018

support rate limiting for the service fabric provider #23

Open

ldez approved these changes Mar 21, 2018

View reviewed changes

lawrencegripper added 3 commits March 21, 2018 21:08

WIP on making SF labels match docker provider

eae6ce0

Use buildConfiguration method for sf label tests

9958721

Split existings sf updateConfig test

0c5e2c4

lawrencegripper and others added 11 commits March 21, 2018 21:08

Added tests for existing labels. Brought existing label names in line…

107e6dd

… with labels file.

Continued refactoring to explicit functions. Moved to using full labe…

73764ee

…l name not suffix

Added gocyclo ignore

96481b9

Completed migration of existing template to explicit methods

1a636d2

Added additional header labels to sf provider

0e13c7b

Fix for template bug passing name not service struct

297f77c

Update traefik sf dependancy

de4264c

Move state check to func. Tidy files and functions.

2a64272

Added option to disable label overrides via propertymanager

c21a1f5

Bug: Fix typo on extensionKey

efb59fd

refactor: review.

1321ef7

ldez force-pushed the addlabels branch 2 times, most recently from a924bdd to 9693d1b Compare March 21, 2018 20:16

refactor: review.

826135b

ldez force-pushed the addlabels branch from 9693d1b to 826135b Compare March 21, 2018 20:23

ldez merged commit a0b2008 into master Mar 22, 2018

ldez deleted the addlabels branch March 22, 2018 09:39

ldez mentioned this pull request Mar 22, 2018

Update Service Fabric backend. traefik/traefik#3064

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SF labels to use explicit methods and add missing labels #16

Update SF labels to use explicit methods and add missing labels #16

lawrencegripper commented Dec 15, 2017

ldez commented Dec 15, 2017 •

edited

Loading

lawrencegripper commented Dec 15, 2017 •

edited

Loading

ldez commented Dec 15, 2017

lawrencegripper commented Dec 15, 2017

lawrencegripper commented Dec 22, 2017

ldez commented Dec 22, 2017

lawrencegripper commented Dec 22, 2017

ldez commented Dec 22, 2017

ldez commented Jan 5, 2018

ldez commented Jan 5, 2018

lawrencegripper commented Jan 5, 2018 •

edited

Loading

lawrencegripper commented Jan 9, 2018

jjcollinge left a comment

jjcollinge Jan 22, 2018

lawrencegripper Jan 24, 2018

jjcollinge Jan 22, 2018

lawrencegripper Jan 24, 2018

jjcollinge Jan 22, 2018

lawrencegripper Jan 24, 2018

jjcollinge Jan 24, 2018

ldez left a comment

jjcollinge commented Mar 22, 2018

		@@ -58,7 +61,7 @@ const tmpl = `
		{{range $replica := $partition.Replicas}}
		{{if isPrimary $replica}}

Update SF labels to use explicit methods and add missing labels #16

Update SF labels to use explicit methods and add missing labels #16

Conversation

lawrencegripper commented Dec 15, 2017

ldez commented Dec 15, 2017 • edited Loading

lawrencegripper commented Dec 15, 2017 • edited Loading

ldez commented Dec 15, 2017

lawrencegripper commented Dec 15, 2017

lawrencegripper commented Dec 22, 2017

ldez commented Dec 22, 2017

lawrencegripper commented Dec 22, 2017

ldez commented Dec 22, 2017

ldez commented Jan 5, 2018

ldez commented Jan 5, 2018

lawrencegripper commented Jan 5, 2018 • edited Loading

lawrencegripper commented Jan 9, 2018

jjcollinge left a comment

Choose a reason for hiding this comment

jjcollinge Jan 22, 2018

Choose a reason for hiding this comment

lawrencegripper Jan 24, 2018

Choose a reason for hiding this comment

jjcollinge Jan 22, 2018

Choose a reason for hiding this comment

lawrencegripper Jan 24, 2018

Choose a reason for hiding this comment

jjcollinge Jan 22, 2018

Choose a reason for hiding this comment

lawrencegripper Jan 24, 2018

Choose a reason for hiding this comment

jjcollinge Jan 24, 2018

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

jjcollinge commented Mar 22, 2018

ldez commented Dec 15, 2017 •

edited

Loading

lawrencegripper commented Dec 15, 2017 •

edited

Loading

lawrencegripper commented Jan 5, 2018 •

edited

Loading