Operator Create Network Area #382

larrytamnjong · 2024-10-21T16:11:50Z

marcin-krystianc · 2024-10-22T09:33:24Z

Consul/Operator.cs

@@ -100,6 +100,34 @@ public class KeyringResponse
        public int NumNodes { get; set; }
    }

+    public class Area


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

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

marcin-krystianc · 2024-10-23T13:23:05Z

Consul/Operator.cs

+        public bool UseTLS { get; set; }
+    }
+
+    public class AreaResponse : Area


Inheritance is not needed here, please remove it.

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.

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

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!

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

marcin-krystianc · 2024-10-23T13:24:09Z

Consul/Interfaces/IOperatorEndpoint.cs

@@ -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);


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

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

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

…g/consuldotnet into create-network-area

marcin-krystianc · 2024-10-25T07:42:12Z

Consul/Operator.cs

+        public bool UseTLS { get; set; }
+    }
+
+    public class AreaResponse : Area


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

marcin-krystianc · 2024-10-25T07:52:36Z

Consul/Operator.cs

+        public async Task<WriteResult<AreaResponse>> CreateArea(Area area, WriteOptions q, CancellationToken ct = default)
+        {
+            var req = await _client.Post<Area, AreaResponse>("/v1/operator/area", area, q).Execute(ct).ConfigureAwait(false);
+            return new WriteResult<AreaResponse>(req, req.Response);


Returning an object, where only the ID field will be initialised, but others will be not - is not great from the usability perspective. (It is confusing, to se a struct which is not fully initialised, you need an extra effort to refer to docs or to implementation to understand why is that)
In the go implementation (https://github.com/hashicorp/consul/blob/main/api/operator_area.go#L91), the Create method returns string, so I guess that what we also should do(i.e. return new WriteResult<string>(req, req.Response.ID)).

Initially I wanted a dedicated struct for response, because I anticipated other fields being added to the response. But since go implementation returns only a single string, we can do it too.

Thank you so much, @marcin-krystianc for the detailed explanation and guidance! 😊 I've made the changes you suggested. Please let me know if there’s anything else I should adjust.

marcin-krystianc · 2024-10-25T08:36:30Z

Consul/Operator.cs

+    public class Area : AreaRequest
+    {
+        /// <summary>
+        /// ID is this identifier for an area (a UUID). This must be left empty


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

marcin-krystianc · 2024-10-25T08:37:09Z

Consul/Operator.cs

@@ -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


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.

Done, thanks.

Consul/Operator.cs

Update documentation comment Co-authored-by: Marcin Krystianc <[email protected]>

marcin-krystianc

Looks good now, thanks for bearing with me.

larrytamnjong added 2 commits October 21, 2024 16:45

add operator area create support (G-Research#381)

dd55f8f

add test for operator create network area (G-Research#381)

e1fe8dc

larrytamnjong mentioned this pull request Oct 22, 2024

add admin partition create API support (#379) #380

Closed

marcin-krystianc reviewed Oct 22, 2024

View reviewed changes

larrytamnjong and others added 4 commits October 22, 2024 10:54

split area structure into request and response

fa3efc0

style: fix whitespaces

ba8cffd

Merge branch 'G-Research:master' into create-network-area

23bcb35

Merge branch 'master' into create-network-area

d7e8c26

marcin-krystianc reviewed Oct 23, 2024

View reviewed changes

larrytamnjong and others added 5 commits October 23, 2024 15:27

remove unnecessary inheritance and update create area result return type

3f4128f

Merge branch 'create-network-area' of https://github.com/larrytamnjon…

9d3d7ff

…g/consuldotnet into create-network-area

added inheritance to area-response structure

6a5f359

style: fix whitespaces

fe6123f

Merge branch 'G-Research:master' into create-network-area

5cb9ed0

marcin-krystianc reviewed Oct 25, 2024

View reviewed changes

refactor area struct usage and response handling in Create method

a0c4986

marcin-krystianc reviewed Oct 25, 2024

View reviewed changes

remove unnecessary comments

97444c3

marcin-krystianc reviewed Oct 25, 2024

View reviewed changes

Consul/Operator.cs Outdated Show resolved Hide resolved

Consul/Operator.cs Outdated Show resolved Hide resolved

larrytamnjong and others added 2 commits October 25, 2024 10:00

Update Consul/Operator.cs

3abb767

Update documentation comment Co-authored-by: Marcin Krystianc <[email protected]>

Update Consul/Operator.cs

e6ac926

Update documentation comment Co-authored-by: Marcin Krystianc <[email protected]>

marcin-krystianc approved these changes Oct 25, 2024

View reviewed changes

marcin-krystianc merged commit c4c0fab into G-Research:master Oct 25, 2024
243 checks passed

larrytamnjong deleted the create-network-area branch October 28, 2024 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operator Create Network Area #382

Operator Create Network Area #382

larrytamnjong commented Oct 21, 2024

marcin-krystianc Oct 22, 2024

larrytamnjong Oct 22, 2024

marcin-krystianc Oct 23, 2024

larrytamnjong Oct 23, 2024

marcin-krystianc Oct 24, 2024

larrytamnjong Oct 24, 2024

marcin-krystianc Oct 25, 2024

marcin-krystianc Oct 23, 2024

marcin-krystianc Oct 25, 2024

marcin-krystianc Oct 25, 2024

marcin-krystianc Oct 25, 2024

larrytamnjong Oct 25, 2024

marcin-krystianc Oct 25, 2024

marcin-krystianc Oct 25, 2024 •

edited

Loading

larrytamnjong Oct 25, 2024

marcin-krystianc left a comment

Operator Create Network Area #382

Operator Create Network Area #382

Conversation

larrytamnjong commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-krystianc Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcin-krystianc left a comment

Choose a reason for hiding this comment

marcin-krystianc Oct 25, 2024 •

edited

Loading