feat: introduce `Namespace` #229

winstxnhdw · 2023-07-31T14:35:56Z

Add v1.7 Enterprise-only Namespace
Add unit tests for Namespace
Apply lint suggestions
Update documentation pertaining to Namespace
Make EnterpriseOnlyFact skippable
Closes Registering a service in a namespace #117

marcin-krystianc

Changes seem to be ok but it would be great if we have a test that makes sure that separation with namespaces actually works.

Consul/Client_Options.cs

marcin-krystianc

Changes seem to be ok but it would be great if we have a test that makes sure that separation with namespaces actually works.

Consul/Client_Options.cs

Consul/Client.cs

marcin-krystianc

Changes for the "namespace" feature are hard to spot due to many changes suggested by linter. I suggest moving the linter-induced changes to a new PR, so we can review it separately.

winstxnhdw · 2023-08-28T18:46:28Z

Tests still fail for Consul version >= 1.10 because enterprise is not supported. Any suggestions on how we can skip these tests?

marcin-krystianc · 2023-08-28T20:36:41Z

Tests still fail for Consul version >= 1.10 because enterprise is not supported. Any suggestions on how we can skip these tests?

Look at the Operator_GetLicense test.

winstxnhdw · 2023-08-28T20:54:42Z

Look at the Operator_GetLicense test.

Hmm.. It still fails for <1.7

Shall I create a new attribute SkippableEnterpriseOnlyFact?

marcin-krystianc · 2023-08-29T12:11:17Z

I think you also need to combine two checks:

a version, because it is 1.7+ version feature (Skip.If(AgentVersion < SemanticVersion.Parse("1.7.0"), $"...");
and whether to execute enterprise tests (SkippableEnterpriseOnlyFact)

I'm not sure, but I don't think it is necessary to implement a new attribute to achieve that.

winstxnhdw · 2023-08-29T12:46:31Z

Currently I don’t think that EnterpriseOnlyFact is skippable.

winstxnhdw · 2023-08-29T14:50:06Z

Yup, just tested. We do need to create a new attribute. For now, I'll just make EnterpriseOnlyFact skippable instead.

marcin-krystianc

The changes look good, just left a few minor comments. I think it would be nice also to write a test that would exercise the namespaces feature in a way that is not yet presented in this PR. For example, we could use KV store (https://developer.hashicorp.com/consul/commands/kv) to show that it is possible to store values in different namespaces and assert that they are not visible to each other.

Consul.Test/NamespaceTest.cs

marcin-krystianc · 2023-08-30T07:53:34Z

Consul/Namespace.cs

+    {
+        public string Name { get; set; }
+
+        public string Description { get; set; } = "";


nit: I'm not sure about this default initialization. Was it causing any trouble when it was null by default? Is there any benefit?

It doesn't seem to cause any trouble when it is null but the Consul server expects an empty string and I am not sure if it could cause unforeseen effects otherwise. See here.

docs/docs/2-guides/1-basic-usage.mdx

docs/docs/2-guides/3-supported-apis.mdx

marcin-krystianc

Thank you for this amazing work. There is just single formatting issue remaining, apart from that all looks really good. Well done 🏆

Consul/Client.cs

winstxnhdw and others added 2 commits July 31, 2023 22:34

feat: introduce Namespace

0b041c0

Merge branch 'master' into namespace

4cc0f50

marcin-krystianc reviewed Aug 1, 2023

View reviewed changes

Consul/Client_Options.cs Show resolved Hide resolved

marcin-krystianc reviewed Aug 1, 2023

View reviewed changes

winstxnhdw and others added 2 commits August 2, 2023 15:15

chore: add notice for enterprise-only features

1447adc

Merge branch 'master' into namespace

85d3d03

mfkl reviewed Aug 4, 2023

View reviewed changes

Consul/Client_Options.cs Outdated Show resolved Hide resolved

winstxnhdw and others added 4 commits August 9, 2023 18:47

Merge branch 'master' into namespace

9bf414e

feat: add Namespace controller

351b98d

Merge branch 'master' into namespace

2f8186f

fix: intialise Namespace correctly

0c32831

marcin-krystianc reviewed Aug 10, 2023

View reviewed changes

Consul/Client.cs Outdated Show resolved Hide resolved

fix: use correct env variable for default namespace

79ec6a4

marcin-krystianc reviewed Aug 11, 2023

View reviewed changes

winstxnhdw and others added 13 commits August 16, 2023 22:00

feat: add overload to use default Query/WriteOptions

9bf6e5b

feat: add test for Namespaces API

103cf5b

chore: remove unused using

acc987f

fix: assign _namespaces

cd4fbb7

fix: skip namespace test if not supported

0cf301e

fix: remove Partition field

b0d882d

chore: sort usings

b8f4d30

Merge branch 'master' into namespace

9e18df6

fix: use DateTime instead of TimeSpan

207d2a6

fix: deleted namespaces are not immediately deleted

a32d82b

fix: ToHashSet is not supported in net461

6274a47

fix: Put request must specify incoming response

b2e85a7

refactor: DeletedAt should be in main Namespace class

2231c57

fix: use EnterpriseOnlyFact attribute instead

d6931cd

refactor: expose correct response

3a689cb

chore: try skipping without a skippable attribute

6a66429

winstxnhdw added 3 commits August 29, 2023 22:53

feat: make EnterpriseOnlyFact skippable

a0812b1

chore: fix name when skipping test

b783ee9

docs: add documentation for Namespaces

476dadf

marcin-krystianc reviewed Aug 30, 2023

View reviewed changes

winstxnhdw added 2 commits August 30, 2023 21:06

refactor: verify if created namespaces exist

9064700

feat: add Namespace test with KV

0c47061

marcin-krystianc previously approved these changes Aug 30, 2023

View reviewed changes

Consul/Client.cs Outdated Show resolved Hide resolved

chore: fix comment indentation

f50a2ac

winstxnhdw dismissed marcin-krystianc’s stale review via f50a2ac August 30, 2023 23:36

fix: apply dotnet format

6f0f8a1

marcin-krystianc approved these changes Aug 31, 2023

View reviewed changes

marcin-krystianc requested a review from mfkl August 31, 2023 06:36

mfkl reviewed Aug 31, 2023

View reviewed changes

Consul/Client.cs Show resolved Hide resolved

mfkl approved these changes Aug 31, 2023

View reviewed changes

marcin-krystianc merged commit 16419e9 into G-Research:master Aug 31, 2023
147 checks passed

winstxnhdw deleted the namespace branch August 31, 2023 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce `Namespace` #229

feat: introduce `Namespace` #229

winstxnhdw commented Jul 31, 2023 •

edited

Loading

marcin-krystianc left a comment

marcin-krystianc left a comment

marcin-krystianc left a comment

winstxnhdw commented Aug 28, 2023

marcin-krystianc commented Aug 28, 2023

winstxnhdw commented Aug 28, 2023

marcin-krystianc commented Aug 29, 2023

winstxnhdw commented Aug 29, 2023

winstxnhdw commented Aug 29, 2023 •

edited

Loading

marcin-krystianc left a comment

marcin-krystianc Aug 30, 2023

winstxnhdw Aug 30, 2023 •

edited

Loading

marcin-krystianc left a comment

feat: introduce Namespace #229

feat: introduce Namespace #229

Conversation

winstxnhdw commented Jul 31, 2023 • edited Loading

marcin-krystianc left a comment

Choose a reason for hiding this comment

marcin-krystianc left a comment

Choose a reason for hiding this comment

marcin-krystianc left a comment

Choose a reason for hiding this comment

winstxnhdw commented Aug 28, 2023

marcin-krystianc commented Aug 28, 2023

winstxnhdw commented Aug 28, 2023

marcin-krystianc commented Aug 29, 2023

winstxnhdw commented Aug 29, 2023

winstxnhdw commented Aug 29, 2023 • edited Loading

marcin-krystianc left a comment

Choose a reason for hiding this comment

marcin-krystianc Aug 30, 2023

Choose a reason for hiding this comment

winstxnhdw Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

marcin-krystianc left a comment

Choose a reason for hiding this comment

feat: introduce `Namespace` #229

feat: introduce `Namespace` #229

winstxnhdw commented Jul 31, 2023 •

edited

Loading

winstxnhdw commented Aug 29, 2023 •

edited

Loading

winstxnhdw Aug 30, 2023 •

edited

Loading