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

Jump to bottom

CFE-679: Add user defined tags to the created DNS resources #874

Merged
merged 2 commits into from Feb 17, 2023
Merged

CFE-679: Add user defined tags to the created DNS resources #874

merged 2 commits into from Feb 17, 2023

Conversation

bharath-b-rh
Copy link
Contributor

Enhancement proposed for Azure tags support in OCP, requires cluster-ingress-operator to add azure userTags available in the status sub resource of infrastructure CR, to the azure DNS resource created.

@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 Jan 18, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 18, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@bharath-b-rh
Copy link
Contributor Author

/label tide/merge-method-squash

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 24, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2023
@bharath-b-rh bharath-b-rh marked this pull request as ready for review February 6, 2023 12:14
@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 Feb 6, 2023
@openshift-ci openshift-ci bot requested review from gcs278 and miheer February 6, 2023 12:15
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2023
@bharath-b-rh
Copy link
Contributor Author

/cc @Miciah

@openshift-ci openshift-ci bot requested a review from Miciah February 8, 2023 11:42
@Miciah
Copy link
Contributor

Miciah commented Feb 8, 2023

/assign

@bharath-b-rh
Copy link
Contributor Author

/remove-label tide/merge-method-squash

@openshift-ci openshift-ci bot removed the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 10, 2023
@bharath-b-rh
Copy link
Contributor Author

/retest

@omertuc
Copy link
Contributor

omertuc commented Feb 13, 2023 •
edited

/test e2e-aws-ovn-single-node

Checking to see if the previous failure was resolved

@bharath-b-rh
Copy link
Contributor Author

/test e2e-gcp-operator

@bharath-b-rh
Copy link
Contributor Author

/test e2e-gcp-operator

@candita
Copy link
Contributor

candita commented Feb 15, 2023

/assign

Miciah
Miciah reviewed Feb 15, 2023
View reviewed changes
Copy link
Contributor

@Miciah Miciah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments; nothing that should block this PR.
/lgtm
/hold
for Candace since she is also assigned as a review.

pkg/dns/azure/client/client.go Show resolved Hide resolved
pkg/dns/azure/dns.go Outdated Show resolved Hide resolved
pkg/dns/azure/dns_test.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 15, 2023
candita
candita suggested changes Feb 15, 2023
View reviewed changes
Copy link
Contributor

@candita candita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good, but I wonder if a privateRecordSet is supposed to be marked as "owned" ?

pkg/dns/azure/dns_test.go Outdated Show resolved Hide resolved
pkg/dns/azure/dns.go Outdated Show resolved Hide resolved
pkg/dns/azure/client/client.go Show resolved Hide resolved
pkg/dns/azure/dns_test.go Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2023
@bharath-b-rh
Copy link
Contributor Author

/test e2e-azure-ovn

@bharath-b-rh
Copy link
Contributor Author

/test e2e-aws-ovn-single-node

@candita
Copy link
Contributor

candita commented Feb 16, 2023

Is this a one-time failure?

CONT TestAll/parallel/TestManagedDNSToUnmanagedDNSIngressController
unmanaged_dns_test.go:177: verifying conditions on DNSRecord zone {ID:/subscriptions/d38f1e38-4bed-438e-b227-833f997adf6a/resourceGroups/ci-op-m557btv6-04a70-l9s52-rg/providers/Microsoft.Network/privateDnsZones/ci-op-m557btv6-04a70.ci.azure.devcluster.openshift.com Tags:map[]}
unmanaged_dns_test.go:177: DNSRecord zone expected to have status=Unknown but got status=True

/test e2e-azure-operator

@candita
Copy link
Contributor

candita commented Feb 16, 2023

Presuming the test stops failing...
/lgtm
/approved

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2023
@Miciah
Copy link
Contributor

Miciah commented Feb 16, 2023

I think TestManagedDNSToUnmanagedDNSIngressController is racy; the test should have a polling loop where it checks the DNSRecord CR's status in case the dns controller needs more time to upsert the record and update status.

@candita
Copy link
Contributor

candita commented Feb 16, 2023

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2023
@candita
Copy link
Contributor

candita commented Feb 16, 2023

/approved

@CFields651
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Feb 16, 2023
@lihongan
Copy link
Contributor

/label qe-approved
Thanks @jinyunma for checking this in pre-merge testing (see https://issues.redhat.com/browse/CORS-2507)

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Feb 17, 2023
@bharath-b-rh
Copy link
Contributor Author

/test e2e-azure-operator

@xenolinux
Copy link

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Feb 17, 2023
@bharath-b-rh
Copy link
Contributor Author

/test e2e-azure-operator

2 similar comments
@bharath-b-rh
Copy link
Contributor Author

/test e2e-azure-operator

@bharath-b-rh
Copy link
Contributor Author

/test e2e-azure-operator

@candita
Copy link
Contributor

candita commented Feb 17, 2023

Asked about today's CI failure in https://redhat-internal.slack.com/archives/CBN38N3MW/p1676645935272899

@candita
Copy link
Contributor

candita commented Feb 17, 2023

/retest-required

@candita
Copy link
Contributor

candita commented Feb 17, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita

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 Feb 17, 2023
@candita
Copy link
Contributor

candita commented Feb 17, 2023

level=info msg=Cluster operator insights SCAAvailable is False with Forbidden: Failed to pull SCA certs from https://api.openshift.com/api/accounts_mgmt/v1/certificates: OCM API https://api.openshift.com/api/accounts_mgmt/v1/certificates returned HTTP 403: {"code":"ACCT-MGMT-11","href":"/api/accounts_mgmt/v1/errors/11","id":"11","kind":"Error","operation_id":"afa5bc77-b571-4c71-87bd-ce09c3f5e513","reason":"Account with ID 2DUeKzzTD9ngfsQ6YgkzdJn1jA4 denied access to perform create on Certificate with HTTP call POST /api/accounts_mgmt/v1/certificates"}

level=error msg=Cluster operator kube-controller-manager Degraded is True with MissingStaticPodController_SyncError::StaticPods_Error: MissingStaticPodControllerDegraded: static pod lifecycle failure - static pod: "kube-controller-manager" in namespace: "openshift-kube-controller-manager" for revision: 6 on node: "ci-op-bg0rzsb7-04a70-gmtcd-master-2" didn't show up, waited: 3m0s

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 17, 2023

@bharath-b-rh: all tests passed!

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.

@openshift-merge-robot openshift-merge-robot merged commit a2f4859 into openshift:master Feb 17, 2023
@bharath-b-rh bharath-b-rh deleted the cfe-679 branch February 18, 2023 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Miciah Miciah Miciah left review comments

@candita candita candita requested changes

@gcs278 gcs278 Awaiting requested review from gcs278

@miheer miheer Awaiting requested review from miheer

Assignees

@Miciah Miciah

@candita candita

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@bharath-b-rh @Miciah @omertuc @candita @CFields651 @lihongan @xenolinux @openshift-merge-robot