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

Add Initial BlueCat Provider Support #1506

Merged
merged 1 commit into from
Mar 18, 2021
Merged

Add Initial BlueCat Provider Support #1506

merged 1 commit into from
Mar 18, 2021

Conversation

seanmalloy
Copy link
Member

@seanmalloy seanmalloy commented Apr 10, 2020 •
edited
Loading

The new BlueCat provider uses the BlueCat API Gateway(REST API). Not
the legacy XML based BlueCat API. This implements feature request #1198.

https://github.com/bluecatlabs/gateway-workflows

Final Count Down:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 10, 2020
@seanmalloy
Copy link
Member Author

/hold

This is a work in progress. Do not merge yet.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2020
@seanmalloy
Copy link
Member Author

FYI ... @vinny-sabatini @sanbornick @linit01 @timcurless here is the WIP pull request to add the BlueCat provider support.

@seanmalloy seanmalloy mentioned this pull request Apr 10, 2020
Closed
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2020
@seanmalloy
Copy link
Member Author

I'm starting to work on this again. The next step is to get REST API DNS TXT record support added to the BlueCat API Gateway. See work in progress PR bluecatlabs/gateway-workflows#105.

cc: @brross @vinny-sabatini @sanbornick

@seanmalloy
Copy link
Member Author

Update ...

This pull request is not dead yet. :-)

We are in the process of getting a BlueCat API Gateway environment setup, so that we can test the REST API for DNS TXT record CRUD operations. Hoping to have the environment ready this week for testing. After that we can get back to finishing off the Go code in this PR.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2020
@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 9, 2020
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 9, 2020
@seanmalloy
Copy link
Member Author

/check-cla

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 9, 2020
@seanmalloy
Copy link
Member Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 9, 2020
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 10, 2020
@seanmalloy
Copy link
Member Author

/uncc @hjacobs @linki

@seanmalloy
Copy link
Member Author

/skip

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2021
vinny-sabatini
vinny-sabatini suggested changes Mar 17, 2021
View reviewed changes
Copy link
Contributor

@vinny-sabatini vinny-sabatini left a comment

Choose a reason for hiding this comment

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

There are some outdated TODO comments that have been fixed and can be cleaned up to avoid confusion. Other than that I think this is good for a v1 that can be iterated on.

docs/tutorials/bluecat.md Outdated Show resolved Hide resolved
provider/bluecat/bluecat.go Outdated Show resolved Hide resolved
provider/bluecat/bluecat.go Outdated Show resolved Hide resolved
provider/bluecat/bluecat.go Outdated Show resolved Hide resolved
provider/bluecat/bluecat.go Outdated Show resolved Hide resolved
provider/bluecat/bluecat.go Outdated Show resolved Hide resolved
provider/bluecat/bluecat.go Outdated Show resolved Hide resolved
provider/bluecat/bluecat.go Outdated Show resolved Hide resolved
provider/bluecat/bluecat.go Outdated Show resolved Hide resolved
provider/bluecat/bluecat.go Outdated Show resolved Hide resolved
@seanmalloy
Copy link
Member Author

@vinny-sabatini suggested changes have been made. Let know know if you think it is good now, and I will squash commits. Thanks!

vinny-sabatini
vinny-sabatini approved these changes Mar 17, 2021
View reviewed changes
Copy link
Contributor

@vinny-sabatini vinny-sabatini left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2021
@vinny-sabatini
Copy link
Contributor

It looks like removing one of the TODO comments left two whitespaces on line 224 of provider/bluecat/bluecat.go and that's failing the gofmt. If you can clean that up I think it is then good to go.
/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2021
@timcurless @seanmalloy
Add Initial BlueCat Provider Support
e20aea4 
The new BlueCat provider uses the BlueCat API Gateway(REST API). Not
the legacy XML based BlueCat API.

https://github.com/bluecatlabs/gateway-workflows
@seanmalloy
Copy link
Member Author

It looks like removing one of the TODO comments left two whitespaces on line 224 of provider/bluecat/bluecat.go and that's failing the gofmt. If you can clean that up I think it is then good to go.
/lgtm cancel

I believe gofmt is fixed now. I also squashed commits. @vinny-sabatini let me know if any other changes are needed. Thanks!

vinny-sabatini
vinny-sabatini approved these changes Mar 17, 2021
View reviewed changes
Copy link
Contributor

@vinny-sabatini vinny-sabatini left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2021
@seanmalloy
Copy link
Member Author

seanmalloy commented Mar 17, 2021 •
edited
Loading

This is the initial support for the BlueCat DNS provider. Myself and @vinny-sabatini plan to support and improve this provider after this initial pull request is merged. Therefore we have listed ourselves in the OWNERS file for this provider.

We verified the basic functionality is working in our non-production environment.

/assign @Raffo @njuettner

@Raffo
Copy link
Contributor

Raffo commented Mar 18, 2021

I have only skimmed this PR and it sounds good as approach that @vinny-sabatini and @seanmalloy will own this and its improvements. I will avoid pointing out TODOs and a few other missing things and leave the improvements of this provider to you.

@Raffo
Copy link
Contributor

Raffo commented Mar 18, 2021

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Raffo, seanmalloy, vinny-sabatini

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 Mar 18, 2021
@k8s-ci-robot k8s-ci-robot merged commit 87960f9 into kubernetes-sigs:master Mar 18, 2021
@vinny-sabatini vinny-sabatini mentioned this pull request Dec 30, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinny-sabatini vinny-sabatini vinny-sabatini approved these changes

Assignees

@Raffo Raffo

@njuettner njuettner

@vinny-sabatini vinny-sabatini

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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. new-provider ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@seanmalloy @k8s-ci-robot @vinny-sabatini @fejta-bot @Raffo @njuettner @timcurless