[PR-156 follow-up] Generate endpoints hostnames if go-template is specified #160

ideahitme · 2017-04-18T10:20:02Z

ideahitme · 2017-04-18T12:27:21Z

hjacobs · 2017-04-18T13:11:12Z

pkg/apis/externaldns/types.go

@@ -81,5 +82,6 @@ func (cfg *Config) ParseFlags(args []string) error {
 	flags.StringVar(&cfg.Registry, "registry", "noop", "type of registry for ownership: <noop|txt>")
 	flags.StringVar(&cfg.RecordOwnerID, "record-owner-id", "", "id for keeping track of the managed records")
 	flags.StringVar(&cfg.TXTPrefix, "txt-prefix", "", `prefix assigned to DNS name of the associated TXT record; e.g. for --txt-prefix=abc_ [CNAME example.org] <-> [TXT abc_example.org]`)
+	flags.StringVar(&cfg.FqdnTemplate, "fqdn-template", "", "template to create FQDN")


maybe add example template usage here

hjacobs · 2017-04-18T13:54:30Z

LGTM

szuecs · 2017-04-18T13:57:49Z

source/ingress.go

@@ -17,6 +17,10 @@ limitations under the License.
 package source

 import (
+	"bytes"
+	"html/template"


Why not text/template?

szuecs · 2017-04-18T14:00:09Z

source/ingress.go

@@ -48,23 +68,49 @@ func (sc *ingressSource) Endpoints() ([]*endpoint.Endpoint, error) {
 	endpoints := []*endpoint.Endpoint{}

 	for _, ing := range ingresses.Items {
+		// Check controller annotation to see if we are responsible.
+		controller, exists := ing.Annotations[controllerAnnotationKey]


instead of "exists" use "ok", it's the normal pattern used in the go community

I just copy pasted this :D but yeah ok is more standard

szuecs · 2017-04-18T14:02:29Z

source/ingress.go

-		return endpoints
+	var buf bytes.Buffer
+
+	if sc.fqdntemplate.Execute(&buf, ing) == nil { //TODO(ideahitme): if error is present skip or abort ?


Log the error, return the error or handle the error, but not silently ignore it.
A user will do errors and should be able to understand that it's a wrong go template which could not be rendered.

yes, that's why there is TODO, we are lacking logging where it should be logged in multiple places, but I will add the logging here :)

szuecs · 2017-04-18T14:08:31Z

source/service.go

@@ -17,6 +17,10 @@ limitations under the License.
 package source

 import (
+	"bytes"
+	"html/template"


use text/template instead

szuecs · 2017-04-18T14:09:56Z

source/service.go

 }

 // NewServiceSource creates a new serviceSource with the given client and namespace scope.
-func NewServiceSource(client kubernetes.Interface, namespace string, compatibility bool) Source {
+func NewServiceSource(client kubernetes.Interface, namespace string, compatibility bool, fqdntemplate string) (Source, error) {


Personally I would like to have the signature

func NewServiceSource(client kubernetes.Interface, namespace, fqdntemplate string, compatibility bool) (Source, error) {

yeah makes sense :)

szuecs · 2017-04-18T14:13:21Z

source/service.go

-		return nil
+	var buf bytes.Buffer
+
+	if sc.fqdntemplate.Execute(&buf, svc) == nil { //TODO(ideahitme): if error is present skip or abort ?


as above please log or handle the error not silently drop it

ideahitme · 2017-04-18T14:34:00Z

@hjacobs @szuecs thanks for valuable comments. please take a look again

szuecs · 2017-04-18T14:58:22Z

👍

mikkeloscar · 2017-04-18T16:52:29Z

CHANGELOG.md

@@ -1,5 +1,5 @@
 Features:
-
+  - Generate DNS Name from template for services/ingress if annotation is missing but `--fqdntemplate` is specified


Flag is missing a - :)

…cified (kubernetes-sigs#160) * add --fqdn-template * add missing , * gofmt * no endpoint creation on empty fqdntemplate * improve test coverage * gofmt simple on service_test.go and ingress_test.go * import package order changed * gofmt * refactor to generate template in the source init * refactor for err handling * fix service tests * fix wrong check, check for priorities, mate > template * fix tests, check for controller annotation in the right place * add to changelog * add flag description, improve testing, reorganize imports * review changes: log the error, use text/template, change func interface

UnsignedLong and others added 10 commits April 13, 2017 12:17

add --fqdn-template

58d531e

add missing ,

e76c89f

gofmt

ddbb971

no endpoint creation on empty fqdntemplate

832a7ba

improve test coverage

cd932dd

gofmt simple on service_test.go and ingress_test.go

d9ec6bf

import package order changed

527b2b0

gofmt

ff913e0

refactor to generate template in the source init

eba7543

refactor for err handling

3583ed7

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 18, 2017

ideahitme self-assigned this Apr 18, 2017

ideahitme added the work-in-progress label Apr 18, 2017

ideahitme mentioned this pull request Apr 18, 2017

PR for issues #142 and #143 #156

Closed

ideahitme added 3 commits April 18, 2017 13:44

fix service tests

f767af7

fix wrong check, check for priorities, mate > template

82e3b54

fix tests, check for controller annotation in the right place

0fe4534

ideahitme requested a review from linki April 18, 2017 12:27

ideahitme added awaiting review and removed work-in-progress labels Apr 18, 2017

ideahitme added 2 commits April 18, 2017 14:59

add to changelog

5cd3e1f

rebase on master

7cb6a68

hjacobs reviewed Apr 18, 2017

View reviewed changes

add flag description, improve testing, reorganize imports

0eb5cb4

hjacobs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2017

szuecs reviewed Apr 18, 2017

View reviewed changes

review changes: log the error, use text/template, change func interface

3499069

ideahitme removed the awaiting review label Apr 18, 2017

hjacobs merged commit 25eef91 into master Apr 18, 2017

mikkeloscar reviewed Apr 18, 2017

View reviewed changes

This was referenced Apr 19, 2017

Support DNS records for ingresses without rules #143

Closed

Add a way to provide external-dns with a template to generate service FQDNs #142

Closed

ideahitme deleted the pr-156-follow-up branch April 19, 2017 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PR-156 follow-up] Generate endpoints hostnames if go-template is specified #160

[PR-156 follow-up] Generate endpoints hostnames if go-template is specified #160

ideahitme commented Apr 18, 2017

ideahitme commented Apr 18, 2017

hjacobs Apr 18, 2017

ideahitme Apr 18, 2017

hjacobs commented Apr 18, 2017

szuecs Apr 18, 2017

szuecs Apr 18, 2017

ideahitme Apr 18, 2017

szuecs Apr 18, 2017 •

edited

Loading

ideahitme Apr 18, 2017

szuecs Apr 18, 2017

szuecs Apr 18, 2017

ideahitme Apr 18, 2017

szuecs Apr 18, 2017

ideahitme commented Apr 18, 2017

szuecs commented Apr 18, 2017

mikkeloscar Apr 18, 2017

		@@ -1,5 +1,5 @@
		Features:

		- Generate DNS Name from template for services/ingress if annotation is missing but `--fqdntemplate` is specified

[PR-156 follow-up] Generate endpoints hostnames if go-template is specified #160

[PR-156 follow-up] Generate endpoints hostnames if go-template is specified #160

Conversation

ideahitme commented Apr 18, 2017

ideahitme commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjacobs commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szuecs Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ideahitme commented Apr 18, 2017

szuecs commented Apr 18, 2017

Choose a reason for hiding this comment

szuecs Apr 18, 2017 •

edited

Loading