-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 the provider for the Tencent Cloud. #2630
Add the provider for the Tencent Cloud. #2630
Conversation
Welcome @Hyzhou! |
@Raffo Would you like to review this PR. Thank you. |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
I need it so much。 |
you can use it now. |
thx |
@Hyzhou only support Tencent Cloud(TKE)? |
It is a driver support for Tencent Cloud, and test in TKE Clusters. |
@Hyzhou example, I want to support DNSPod in EKS(Other Cloud K8s) Clusters, this driver maybe not work. |
d2ba0b9
to
a6d6e64
Compare
41b88c6
to
10fad34
Compare
/sig cloud-provider |
@Raffo @njuettner @seanmalloy Would you like to review this PR. Thank you. |
/assign @szuecs |
provider/tencentcloud/dnspod.go
Outdated
} | ||
for _, record := range records { | ||
if *record.Type == "TXT" && strings.HasPrefix(*record.Value, "heritage=") { | ||
record.Value = common.StringPtr(fmt.Sprintf("\"%s\"", *record.Value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit nicer would be:
record.Value = common.StringPtr(fmt.Sprintf(`"%s"`, *record.Value))
provider/tencentcloud/dnspod.go
Outdated
for _, endpoint := range endpoints { | ||
for _, target := range endpoint.Targets { | ||
if endpoint.RecordType == "TXT" && strings.HasPrefix(target, "\"heritage=") { | ||
target = strings.Trim(target, "\"") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just scrolling the code a bit and this line and the above looks a bit weird to me
provider/tencentcloud/dnspod.go
Outdated
for _, record := range recordGroup.RecordList { | ||
key := *record.Type + ":" + *record.Name + "." + *recordGroup.Domain.Name | ||
if *record.Name == TencentCloudEmptyPrefix { | ||
key = *record.Type + ":" + *recordGroup.Domain.Name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't need to dereference pointers here (ibmcloud seems to also does it, but all others not) and basically all over the place. See also the inmemory provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I just not understand only this feedback. I wants to group the target by dnsname and type here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sorry!
You can use for example record.Name without prepending *
.
It's different from C or C++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key := record.Type + ":" + record.Name + "." + recordGroup.Domain.Name
like this? it looks wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes like that and in general I prefer fmt.Sprintf for string concatenation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
record.Type is *string type. remove the *
make go language syntax error.
fmt is ok, I already change to fmt.Sprintf
func NewTencentCloudProvider(domainFilter endpoint.DomainFilter, zoneIDFilter provider.ZoneIDFilter, configFile string, zoneType string, dryRun bool) (*TencentCloudProvider, error) { | ||
cfg := tencentCloudConfig{} | ||
if configFile != "" { | ||
contents, err := ioutil.ReadFile(configFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.ReadFile.
if configFile != "" { | ||
contents, err := ioutil.ReadFile(configFile) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to read Tencent Cloud config file '%s': %v", configFile, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use always:
fmt.Errorf("....: %w", err)
instead of %v
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, the above all points. I will check it soon.
t.Errorf("Incorrect number of records: %d", len(endpoints)) | ||
} | ||
for _, endpoint := range endpoints { | ||
t.Logf("Endpoint for %++v", *endpoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%++v -> %+v
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small Go code things here and there that requires fixes.
Other than that we have to discuss in maintainers meeting if we merge all provider PRs or if we have it on hold as we have since more than a half year.
/remove-lifecycle rotten |
/assign @Raffo |
@Raffo to run the tests looks fine to me |
523ed7b
to
dce3174
Compare
I approved tests to run, deferring to @szuecs for the final review and call on merge. |
I see this in the CI build:
there's also a conflict with the go.mod now. Can you please fix that @Hyzhou ? |
I get it |
17cad62
to
4fd48e4
Compare
I already add the lisence header. and rebase the code |
Approved CI again. |
Sorry, I will run it local. |
Signed-off-by: misakazhou <[email protected]>
4fd48e4
to
a2e7ffc
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Hyzhou, szuecs 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 |
Signed-off-by: misakazhou [email protected]
Description
Add the provider for tencent cloud.
PrivateDNS service provides intranet DNS resolution.
DNSPod service provides internet DNS resolution.
Unit tests and document is updated for the tencent cloud provider.
Any issue about the tencent cloud provider can ask @Hyzhou for help and upgrade.
Fixes #ISSUE
Checklist