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.

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

Add support for private DNS server to PowerVS #6157

Merged

Conversation

hamzy
Copy link
Contributor

@hamzy hamzy commented Jul 22, 2022

This adds support for a private DNS server to PowerVS by
conditionally creating resources based on:
publish: Internal
in the install-config.yaml

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2022
@openshift-ci openshift-ci bot requested review from AnnaZivkovic and jstuever July 22, 2022 11:11
@hamzy hamzy force-pushed the add-powervs-private-dns-1 branch 4 times, most recently from b10c652 to b130f2b Compare July 28, 2022 13:22
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2022
@mjturek
Copy link
Contributor

mjturek commented Jul 28, 2022

Looking good so far! Left some comments. Also I think we should adjust commit message to something like:

"Power VS: Conditionally create VPC VM for proxy DNS"

@hamzy hamzy force-pushed the add-powervs-private-dns-1 branch from b130f2b to 8fc8a0c Compare July 28, 2022 18:59
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2022
@hamzy hamzy force-pushed the add-powervs-private-dns-1 branch 3 times, most recently from 14459e5 to a84b851 Compare July 29, 2022 17:50
@mjturek
Copy link
Contributor

mjturek commented Aug 2, 2022

LGTM - can you remove WIP?

@hamzy hamzy changed the title WIP: Add support for private DNS server to PowerVS Add support for private DNS server to PowerVS Aug 2, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2022
@hamzy hamzy force-pushed the add-powervs-private-dns-1 branch 2 times, most recently from 5008219 to 02f15d3 Compare August 8, 2022 19:21
@hamzy
Copy link
Contributor Author

hamzy commented Aug 9, 2022

/retest-required

@mjturek
Copy link
Contributor

mjturek commented Aug 18, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2022
@hamzy hamzy force-pushed the add-powervs-private-dns-1 branch from 02f15d3 to d9cf41e Compare August 21, 2022 20:35
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2022
@hamzy hamzy removed the request for review from jstuever August 21, 2022 20:37
@hamzy hamzy requested review from mjturek and removed request for AnnaZivkovic August 21, 2022 20:37
@hamzy
Copy link
Contributor Author

hamzy commented Aug 21, 2022

/retest

@hamzy
Copy link
Contributor Author

hamzy commented Aug 21, 2022

/retest-required

1 similar comment
@hamzy
Copy link
Contributor Author

hamzy commented Aug 22, 2022

/retest-required

@clnperez
Copy link
Contributor

@rna-afk can we get a review on this one?

@hamzy hamzy force-pushed the add-powervs-private-dns-1 branch from d9cf41e to 1ea67bc Compare August 23, 2022 19:41
This adds support for a private DNS server to PowerVS by
conditionally creating resources based on:
   publish: Internal
in the install-config.yaml
@hamzy hamzy force-pushed the add-powervs-private-dns-1 branch from 1ea67bc to b98d774 Compare August 23, 2022 20:29
@hamzy hamzy requested review from rna-afk and mjturek and removed request for mjturek and rna-afk August 23, 2022 21:09
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 24, 2022

@hamzy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ovirt b98d774 link false /test e2e-ovirt
ci/prow/e2e-openstack b98d774 link false /test e2e-openstack

Full PR test history. Your PR dashboard.

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.

@hamzy
Copy link
Contributor Author

hamzy commented Aug 24, 2022

/retest-required

@mjturek
Copy link
Contributor

mjturek commented Aug 25, 2022

would you be opposed to moving VM creation into the power_network module rather than dns?

It would simplify using the VM as a proxy DHCP service by allowing the dhcp service resouce to directly read the IP from the proxy VM after it's created rather than relying on an output

EDIT: Discussed offline and decided to keep PR as is.

@mjturek
Copy link
Contributor

mjturek commented Aug 26, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2022
@hamzy hamzy requested review from rna-afk and removed request for mjturek August 26, 2022 21:01
@rna-afk
Copy link
Contributor

rna-afk commented Aug 29, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rna-afk

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2022
@openshift-merge-robot openshift-merge-robot merged commit 0792ab3 into openshift:master Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants