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

Allow using grpc dns target for interdomain scenarious #1571

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

denis-tingaikin
Copy link
Member

@denis-tingaikin denis-tingaikin commented Dec 25, 2023

Description

Issue link

Closes #1507
Closes networkservicemesh/deployments-k8s#5435
Closes networkservicemesh/cmd-registry-proxy#1
Closes networkservicemesh/cmd-registry-proxy#2
Closes networkservicemesh/deployments-k8s#9318
Closes networkservicemesh/deployments-k8s#8343

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionality
  • Documentation
  • Refactoring
  • CI

Copy link

codecov bot commented Dec 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@d76b20e). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1571   +/-   ##
=======================================
  Coverage        ?   67.09%           
=======================================
  Files           ?      261           
  Lines           ?    12347           
  Branches        ?        0           
=======================================
  Hits            ?     8284           
  Misses          ?     3535           
  Partials        ?      528           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@denis-tingaikin denis-tingaikin force-pushed the use-dns-target branch 3 times, most recently from 3b20292 to c932ea9 Compare January 15, 2024 23:50
Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com>
Signed-off-by: Denis Tingaikin <denis.tingajkin@xored.com>
Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com>
require.Equal(t, name+"@"+domains[2].Name, list[0].Name)
}

func TestXX(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add some require in this test

if b.domain.Registry != nil {
require.NoError(b.t, AddSRVEntry(b.dnsResolver, b.name, dnsresolve.DefaultRegistryService, CloneURL(b.domain.Registry.URL)))
func (b *Builder) addDNSRecords() {
if b.domain.DNSServer == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add some warnings here? If DNSServer is not set

@denis-tingaikin denis-tingaikin changed the title Use dns target for interdomain scenarious Allow using grpc dns target for interdomain scenarious Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants