Skip to content

Conversation

@larrytamnjong
Copy link
Contributor

@larrytamnjong larrytamnjong commented Oct 8, 2024

#356 - another take

@larrytamnjong larrytamnjong marked this pull request as ready for review October 9, 2024 02:24
@larrytamnjong
Copy link
Contributor Author

@marcin-krystianc

@marcin-krystianc
Copy link
Contributor

Hi @larrytamnjong ,

please have a look at test errors, e.g.:

[xUnit.net 00:00:25.15]         --- End of stack trace from previous location ---
  Passed Consul.Test.CoordinateTest.Coordinate_GetNode [3 ms]
Error: Consul.Test.DiscoveryChainTest.DiscoveryChaing_Get: Assert.Equal() Failure
  Failed Consul.Test.DiscoveryChainTest.DiscoveryChaing_Get [21 ms]
  Error Message:
   Assert.Equal() Failure
          ↓ (pos 0)
Expected: https
Actual:   tcp
          ↑ (pos 0)
  Stack Trace:
     at Consul.Test.DiscoveryChainTest.DiscoveryChaing_Get() in D:\a\consuldotnet\consuldotnet\Consul.Test\DiscoveryChainTest.cs:line 83
--- End of stack trace from previous location ---

@larrytamnjong
Copy link
Contributor Author

Hi @larrytamnjong ,

please have a look at test errors, e.g.:

[xUnit.net 00:00:25.15]         --- End of stack trace from previous location ---
  Passed Consul.Test.CoordinateTest.Coordinate_GetNode [3 ms]
Error: Consul.Test.DiscoveryChainTest.DiscoveryChaing_Get: Assert.Equal() Failure
  Failed Consul.Test.DiscoveryChainTest.DiscoveryChaing_Get [21 ms]
  Error Message:
   Assert.Equal() Failure
          ↓ (pos 0)
Expected: https
Actual:   tcp
          ↑ (pos 0)
  Stack Trace:
     at Consul.Test.DiscoveryChainTest.DiscoveryChaing_Get() in D:\a\consuldotnet\consuldotnet\Consul.Test\DiscoveryChainTest.cs:line 83
--- End of stack trace from previous location ---

Hi @marcin-krystianc when I debug the test locally, the protocol in the response is "https". However, I don't understand why it's "tcp" here. Could you provide some guidance on how to troubleshoot this issue?
image

@larrytamnjong
Copy link
Contributor Author

Hi @larrytamnjong ,
please have a look at test errors, e.g.:

[xUnit.net 00:00:25.15]         --- End of stack trace from previous location ---
  Passed Consul.Test.CoordinateTest.Coordinate_GetNode [3 ms]
Error: Consul.Test.DiscoveryChainTest.DiscoveryChaing_Get: Assert.Equal() Failure
  Failed Consul.Test.DiscoveryChainTest.DiscoveryChaing_Get [21 ms]
  Error Message:
   Assert.Equal() Failure
          ↓ (pos 0)
Expected: https
Actual:   tcp
          ↑ (pos 0)
  Stack Trace:
     at Consul.Test.DiscoveryChainTest.DiscoveryChaing_Get() in D:\a\consuldotnet\consuldotnet\Consul.Test\DiscoveryChainTest.cs:line 83
--- End of stack trace from previous location ---

Hi @marcin-krystianc when I debug the test locally, the protocol in the response is "https". However, I don't understand why it's "tcp" here. Could you provide some guidance on how to troubleshoot this issue? image

@marcin-krystianc , I think I’ve found the issue that was causing the protocol to be "https" locally instead of "tcp." I've made an update to the test, and it should be resolved now. You can please take another look when you have time.

Co-authored-by: Marcin Krystianc <[email protected]>
Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

This PR generally looks good, but I've noticed we've some CI issues due to changes for linux runners. I'll try sorting this out now.

/// </summary>
public interface IDiscoveryChainEndpoint
{
Task<QueryResult<DiscoveryChainResponse>> Get(string name, QueryOptions q, CancellationToken ct = default);
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need overrides for compile-dc
and other parameters (https://developer.hashicorp.com/consul/api-docs/discovery-chain#json-request-body-schema), but that ideally can be implemented in follow-up PR (so we limit the scope of this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay got it 👍

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

Nice 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants