Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Consul.Test/OperatorTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,5 +96,13 @@ public async Task Segment_List()
var segments = await _client.Operator.SegmentList();
Assert.NotEmpty(segments.Response);
}

[EnterpriseOnlyFact]
public async Task Operator_CreateArea()
{
var check = new Area { PeerDatacenter = "dc2", UseTLS = false, RetryJoin = null };
var response = await _client.Operator.CreateArea(check);
Assert.NotNull(response.Response);
}
}
}
2 changes: 2 additions & 0 deletions Consul/Interfaces/IOperatorEndpoint.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ public interface IOperatorEndpoint
Task<QueryResult<ConsulLicense>> GetConsulLicense(string datacenter = "", CancellationToken ct = default);
Task<QueryResult<string[]>> SegmentList(QueryOptions q, CancellationToken cancellationToken = default);
Task<QueryResult<string[]>> SegmentList(CancellationToken cancellationToken = default);
Task<WriteResult<string>> CreateArea(Area area, WriteOptions 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.

The result type should be Task<WriteResult<AreaResponse>>

Copy link
Contributor

Choose a reason for hiding this comment

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

The result type should be Task<WriteResult<AreaResponse>>

When I made that comment I assumed that AreaResponse will contain only an ID field.

Task<WriteResult<string>> CreateArea(Area area, CancellationToken ct = default);

}
}
50 changes: 50 additions & 0 deletions Consul/Operator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,37 @@ public class KeyringResponse
public int NumNodes { get; set; }
}

public class Area
Copy link
Contributor

Choose a reason for hiding this comment

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

Please split it into two structures, one for request and one for response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve just completed it. Please let me know if everything looks good.

{
/// <summary>
/// PeerDatacenter is the peer Consul datacenter that will make up the
/// other side of this network area. Network areas always involve a pair
/// of datacenters: the datacenter where the area was created, and the
/// peer datacenter. This is required.
/// </summary>
public string PeerDatacenter { get; set; }

/// <summary>
/// RetryJoin specifies the address of Consul servers to join to, such as
/// an IPs or hostnames with an optional port number. This is optional.
/// </summary>
public string[] RetryJoin { get; set; }

/// <summary>
/// UseTLS specifies whether gossip over this area should be encrypted with TLS
/// if possible.
/// </summary>
public bool UseTLS { get; set; }
}

public class AreaResponse : Area
Copy link
Contributor

Choose a reason for hiding this comment

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

Inheritance is not needed here, please remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @marcin-krystianc Initially, I considered using inheritance because, in certain cases like the List Specific Network Area response, additional properties such as PeerDatacenter and RetryJoin are returned. I thought that inheriting from the Area class would make the response more flexible for these scenarios.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @marcin-krystianc Initially, I considered using inheritance because, in certain cases like the List Specific Network Area response, additional properties such as PeerDatacenter and RetryJoin are returned. I think inheriting from the Area class will make the response more flexible for these scenarios.

Ah, I see. It is tempting to reuse existing structure definitions for new ones to save some code, but in my opinion, it isn't always the right thing to do. Given that apart from Create Network Area there are other methods like List Network Areas, Update Network Area, and others - we need to plan how we are going to define requests and responses for all of them.
My initial plan was to create a dedicated request and response structure definition for each method. That would give us full control over those structures and would define the interface very clearly from a user perspective (It is more code in the library to write but it is very straightforward).
On the other hand, I looked at the consul implementation of Operator_Area API (https://github.com/hashicorp/consul/blob/main/api/operator_area.go) and maybe it is ok to do it similarly.
It would require fewer structure definitions but I guess the API will be less clear from the user perspective (for example AreaUpdate methods take ID and Area objects, but Area has already an ID but it is not used).

I think I'll leave it to you to decide. Both approaches have pros and cons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the insights @marcin-krystianc ! I initially removed the inheritance and made adjustments based on your initial recommendations, but since you're leaving it up to me, I've added it back. Please let me know what you think!

Copy link
Contributor

Choose a reason for hiding this comment

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

The use of inheritance somewhat bothers me, but maybe unnecessarily :-) The only suggestion I have is to change the names: Area -> AreaRequest and AreaResponse -> Area

{
/// <summary>
/// ID is this identifier for an area (a UUID). This must be left empty
Copy link
Contributor

Choose a reason for hiding this comment

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

With our API, the comment This must be left empty when creating a new area. is not relevant.

/// when creating a new area.
/// </summary>
public string ID { get; set; }
}
public class Operator : IOperatorEndpoint
{
private readonly ConsulClient _client;
Expand Down Expand Up @@ -249,6 +280,25 @@ public Task<QueryResult<string[]>> SegmentList(CancellationToken ct = default)
{
return SegmentList(QueryOptions.Default, ct);
}

/// <summary>
/// CreateArea will create a new network area. The ID in the given structure must
Copy link
Contributor

@marcin-krystianc marcin-krystianc Oct 25, 2024

Choose a reason for hiding this comment

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

here as well it is not relevant for our API -> The ID in the given structure must be empty and a generated ID will be returned on success.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks.

/// be empty and a generated ID will be returned on success.
/// </summary>
public Task<WriteResult<string>> CreateArea(Area area, CancellationToken ct = default)
{
return CreateArea(area, WriteOptions.Default, ct);
}

/// <summary>
/// CreateArea will create a new network area. The ID in the given structure must
/// be empty and a generated ID will be returned on success.
/// </summary>
public async Task<WriteResult<string>> CreateArea(Area area, WriteOptions q, CancellationToken ct = default)
{
var res = await _client.Post<Area, AreaResponse>("/v1/operator/area", area, q).Execute(ct).ConfigureAwait(false);
return new WriteResult<string>(res, res.Response.ID);
}
}

public class ConsulLicense
Expand Down