Bug 1734539: Openstack: Option to set multiple external DNS IP Addresses by iamemilio · Pull Request #2132 · openshift/installer

iamemilio · 2019-07-31T19:51:53Z

Adds an option in the installer that allows users to add the ip addresses of dns servers. These IP's will be added to the nodes subnet.

Bug Fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1734539

iamemilio · 2019-07-31T20:46:43Z

/test e2e-aws

iamemilio · 2019-08-01T12:36:27Z

/test e2e-aws-scaleup-rhel7

mandre · 2019-08-01T13:17:45Z

Shouldn't external_dns be the preferred resolver if it is set?

mandre · 2019-08-01T13:33:45Z

var.external_dns should be moved outside of the array.

iamemilio · 2019-08-01T14:21:05Z

Dependant on #1959

tomassedovic · 2019-08-01T14:37:57Z

/hold

The OpenStack CI is non-voting so I'm putting a hold on this so we don't accidentally merge it before #1959.

The change itself looks good to me.

tomassedovic · 2019-08-05T11:17:05Z

/retest

mandre · 2019-08-06T10:44:57Z

/retest

patrickdillon · 2019-10-25T13:44:57Z

nit: This comment as well as the comments for the rest of the fields except DefaultMachinePlatform is not following the best practice. It should be a complete sentence--rather than having the field name on its own line--and end with a period. Following this convention allows automatic generation of documentation using doc command.

I can open a seperate PR to address this, if thats ok

patrickdillon · 2019-10-25T13:46:36Z

nit: this function signature is getting ridiculously long. this is not a problem with your pr and you don't need to fix it. it is probably like this in many packages other than openstack but I just wanted to take this opportunity to point that out.

agreed, It actually is more a problem with our platform than others. I'm going to make an action item to align more with how aws calls this function

patrickdillon · 2019-10-25T13:47:30Z

it can also be helpful to have a valid test case.

patrickdillon · 2019-10-25T13:48:35Z

code looks good to me.

patrickdillon · 2019-10-25T13:49:56Z

/lgtm

abhinavdahiya · 2019-10-25T13:54:32Z

/approve

would like to see @patrickdillon 2 comments addressed.
correct godoc field comment
valid unit test case for external dnses

but if you think those can be done as follow-up feel free to lift the hold.

/hold

iamemilio · 2019-10-25T14:05:29Z

I will address the comments that do not follow best practice in another pull request. All components related to this feature have been updated to match best practices.

will cancel hold after unit tests pass

iamemilio · 2019-10-25T14:32:39Z

/hold cancel

tomassedovic · 2019-10-25T15:30:36Z

/lgtm

@iamemilio please make the requested changes in a follow-up patch.

openshift-ci-robot · 2019-10-25T15:30:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, iamemilio, mandre, patrickdillon, tomassedovic

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya]

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

openshift-ci-robot · 2019-10-25T15:32:40Z

@iamemilio: All pull requests linked via external trackers have merged. Bugzilla bug 1734539 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1734539: Openstack: Option to set multiple external DNS IP Addresses

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2019-10-25T15:55:50Z

@iamemilio: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-disruptive	266bc21b191b9c3a996f5d625cce08e1228734b5	link	`/test e2e-aws-disruptive`
ci/prow/e2e-aws-scaleup-rhel7	`3d7107a`	link	`/test e2e-aws-scaleup-rhel7`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

diegobranno · 2020-03-20T07:14:10Z

Hi guys,
I have the same problem with RedHat Openshift 4.3.0 on RedHat Openstack 13.
Is it possible to apply what you have described for external DNS? If yes, how should I apply it?
Thanks.
Diego.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 31, 2019

openshift-ci-robot requested review from mtnbikenc and patrickdillon July 31, 2019 19:52

iamemilio changed the title ~~Optionally set multiple external DNS IP Addresses~~ [openstack] Optionally set multiple external DNS IP Addresses Jul 31, 2019

iamemilio changed the title ~~[openstack] Optionally set multiple external DNS IP Addresses~~ Optionally set multiple external DNS IP Addresses Jul 31, 2019

iamemilio changed the title ~~Optionally set multiple external DNS IP Addresses~~ [WIP] Optionally set multiple external DNS IP Addresses Jul 31, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 31, 2019

iamemilio force-pushed the custom-dns branch 2 times, most recently from f79d659 to 59cd73a Compare July 31, 2019 20:21

iamemilio changed the title ~~[WIP] Optionally set multiple external DNS IP Addresses~~ Optionally set multiple external DNS IP Addresses Jul 31, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 31, 2019

iamemilio changed the title ~~Optionally set multiple external DNS IP Addresses~~ Openstack: Option to set multiple external DNS IP Addresses Jul 31, 2019

mandre reviewed Aug 1, 2019

View reviewed changes

iamemilio force-pushed the custom-dns branch from 59cd73a to 7a36972 Compare August 1, 2019 13:28

mandre reviewed Aug 1, 2019

View reviewed changes

iamemilio force-pushed the custom-dns branch 2 times, most recently from c5191e4 to 815cd25 Compare August 1, 2019 14:19

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 1, 2019

iamemilio force-pushed the custom-dns branch from 815cd25 to 412dc8f Compare August 1, 2019 14:23

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2019

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2019

iamemilio force-pushed the custom-dns branch from 412dc8f to 875feac Compare August 9, 2019 14:35

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2019

iamemilio force-pushed the custom-dns branch from 190160d to 3bffa0c Compare October 25, 2019 13:17

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2019

iamemilio force-pushed the custom-dns branch 2 times, most recently from db424be to 7ee6972 Compare October 25, 2019 13:26

patrickdillon reviewed Oct 25, 2019

View reviewed changes

openshift-ci-robot assigned patrickdillon Oct 25, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2019

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 25, 2019

iamemilio force-pushed the custom-dns branch from 7ee6972 to d1b53a8 Compare October 25, 2019 13:58

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2019

simple unit tests to check external dns validation

3d7107a

iamemilio force-pushed the custom-dns branch from d1b53a8 to 3d7107a Compare October 25, 2019 14:01

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2019

openshift-merge-robot merged commit f72704a into openshift:master Oct 25, 2019

iamemilio deleted the custom-dns branch October 25, 2019 16:11

maxwelldb mentioned this pull request Oct 28, 2019

[ShiftStack] Describe how to set multiple external DNS IP addresses openshift/openshift-docs#17668

Closed

Conversation

iamemilio commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamemilio commented Jul 31, 2019

Uh oh!

iamemilio commented Aug 1, 2019

Uh oh!

mandre Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

mandre Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

iamemilio commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomassedovic commented Aug 1, 2019

Uh oh!

tomassedovic commented Aug 5, 2019

Uh oh!

mandre commented Aug 6, 2019

Uh oh!

patrickdillon Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

iamemilio Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

patrickdillon Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

iamemilio Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

patrickdillon Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Oct 25, 2019

Uh oh!

patrickdillon commented Oct 25, 2019

Uh oh!

abhinavdahiya commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamemilio commented Oct 25, 2019

Uh oh!

iamemilio commented Oct 25, 2019

Uh oh!

tomassedovic commented Oct 25, 2019

Uh oh!

openshift-ci-robot commented Oct 25, 2019

Uh oh!

openshift-ci-robot commented Oct 25, 2019

Uh oh!

openshift-ci-robot commented Oct 25, 2019

Uh oh!

diegobranno commented Mar 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

iamemilio commented Jul 31, 2019 •

edited

Loading

iamemilio commented Aug 1, 2019 •

edited

Loading

abhinavdahiya commented Oct 25, 2019 •

edited

Loading