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.

Sign up for GitHub

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

Jump to bottom

Support for openshift routes #1484

Merged
merged 9 commits into from
Apr 20, 2020
Merged

Support for openshift routes #1484

merged 9 commits into from
Apr 20, 2020

Conversation

jgrumboe
Copy link
Contributor

@jgrumboe jgrumboe commented Apr 2, 2020

This implements a new source type "openshift-route" and adds support for openshift routes #706
I've taken the mentioned ocproute implementation from https://github.com/KohlsTechnology/external-dns/blob/timcurless-feature/source/ocproute.go and rebased it to master.

Tested against OpenShift 3.11 and Infoblox as provider.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 2, 2020
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 2, 2020
@jgrumboe
Copy link
Contributor Author

jgrumboe commented Apr 2, 2020

/assign @Raffo

seanmalloy
seanmalloy requested changes Apr 3, 2020
View reviewed changes
source/ocproute.go Show resolved Hide resolved
source/ocproute.go Outdated Show resolved Hide resolved
source/store.go Show resolved Hide resolved
source/store.go Show resolved Hide resolved
@seanmalloy
Copy link
Member

@vinny-sabatini and @sanbornick please review if you have some time.

@jgrumboe
Copy link
Contributor Author

jgrumboe commented Apr 4, 2020

@seanmalloy Thanks for the feedback.
I will work on it after weekend.
Honestly I was copy-pasting from here and there, so mostly there weren't comments in the sources also, but I'll do my very best.

@seanmalloy
Copy link
Member

@jgrumboe let me know if you need any help making the changes. I can try to help.

@jgrumboe jgrumboe requested a review from seanmalloy April 7, 2020 08:25
@vinny-sabatini
Copy link
Contributor

I gave the PR a read through (beginner with Go and not super knowledgeable on this repo) but it looks good to me

@seanmalloy
Copy link
Member

seanmalloy commented Apr 9, 2020 •
edited
Loading

/lgtm

@Raffo this PR is ready for you to review/merge now.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2020
alexcern
alexcern reviewed Apr 9, 2020
View reviewed changes
source/ocproute.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2020
@jgrumboe jgrumboe requested a review from seanmalloy April 9, 2020 13:41
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 10, 2020
@jgrumboe
Copy link
Contributor Author

@seanmalloy @Raffo
Tutorial was added. ltgm please ;)

@seanmalloy
Copy link
Member

/lgtm
/assign @hjacobs @linki @Raffo

@Raffo @hjacobs @linki the PR is ready for review/merge.

I posted in #extnernal-dns and #openshift-dev on k8s slack asking for additional feedback too. Hoping to get input from as many people as possible.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2020
@jgrumboe
Copy link
Contributor Author

Any news?
If someone wants to test it, just pull docker.io/porscheinformatik/external-dns:latest (it's based on 0.7.1)

@seanmalloy
Copy link
Member

@Raffo @linki @hjacobs is there anything we can do to help move this pull request forward?

@hjacobs
Copy link
Contributor

hjacobs commented Apr 20, 2020

/lgtm

@hjacobs
Copy link
Contributor

hjacobs commented Apr 20, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hjacobs, jgrumboe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3ce5701 into kubernetes-sigs:master Apr 20, 2020
@seanmalloy seanmalloy mentioned this pull request Apr 20, 2020
Closed
aliasgharmhowwala pushed a commit to aliasgharmhowwala/external-dns that referenced this pull request Jun 15, 2020
@k8s-ci-robot
Merge pull request kubernetes-sigs#1484 from porscheinformatik/ocproute
5dd3beb 
Support for openshift routes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcern alexcern alexcern left review comments

@hjacobs hjacobs Awaiting requested review from hjacobs

@linki linki Awaiting requested review from linki

@seanmalloy seanmalloy Awaiting requested review from seanmalloy

Assignees

@linki linki

@hjacobs hjacobs

@Raffo Raffo

@seanmalloy seanmalloy

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jgrumboe @seanmalloy @vinny-sabatini @hjacobs @k8s-ci-robot @alexcern @linki @Raffo