features/spec: add sandbox backward compatibility to endpoint spec #268
Conversation
In the initial implementation I wrote for ably-go I included backward compatibility to allow being able to simply specifying `sandbox` rather than having to specify `nonprod:sandbox`, and to ensure this compatibility is included in all client implementations, then I am including it in the spec.
|
@surminus can you check why CI is failing? also can update PR according to comment |
surminus
force-pushed
the
laura/endpoint-sandbox
branch
from
b43938b to
d3b9a2e
Compare
surminus
force-pushed
the
laura/endpoint-sandbox
branch
from
d3b9a2e to
dfb6e62
Compare
|
@sacOO7 I've updated it and fixed CI (it was due to duplicates) |
Great, I will take a look 👍 . |
| @@ -78,6 +78,7 @@ h2(#endpoint-configuration). Client library endpoint configuration | |||
| *** @(REC1b2)@ If the @endpoint@ option is a domain name, determined by it containing at least one period (@.@) then the @primary domain@ is the value of the @endpoint@ option. | |||
| *** @(REC1b3)@ Otherwise, if the @endpoint@ option is a non-production routing policy name of the form @nonprod:[name]@ then the @primary domain@ is @[name].realtime.ably-nonprod.net@. | |||
| *** @(REC1b4)@ Otherwise, if the @endpoint@ option is a production routing policy name of the form @[name]@ then the @primary domain@ is @[name].realtime.ably.net@. | |||
| *** @(REC1b5)@ Otherwise, if the @endpoint@ option is a routing policy name of @sandbox@ then the @primary domain@ is @sandbox.realtime.ably-nonprod.net@. | |||
There was a problem hiding this comment.
Okay, REC1b5 seems the exact interpretation of REC1b3. We don't really need new spec for sandbox since endpoint will be set to nonprod:sandbox and we will get exact same result as per REC1b3
| @@ -95,8 +96,9 @@ h2(#endpoint-configuration). Client library endpoint configuration | |||
| *** @(REC2c2)@ Otherwise, if the @primary domain@ is determined to be an explicit domain name via @(REC1b2)@ then the set of @fallback domains@ is empty. | |||
| *** @(REC2c3)@ Otherwise, if the @primary domain@ is determined to be a non-production routing policy name via @(REC1b3)@ then the set of @fallback domains@ is @[name].a.fallback.ably-realtime-nonprod.com@, @[name].b.fallback.ably-realtime-nonprod.com@, @[name].c.fallback.ably-realtime-nonprod.com@, @[name].d.fallback.ably-realtime-nonprod.com@, @[name].e.fallback.ably-realtime-nonprod.com@. | |||
| *** @(REC2c4)@ Otherwise, if the @primary domain@ is determined to be a production routing policy name via @(REC1b4)@ then the set of @fallback domains@ is @[name].a.fallback.ably-realtime.com@, @[name].b.fallback.ably-realtime.com@, @[name].c.fallback.ably-realtime.com@, @[name].d.fallback.ably-realtime.com@, @[name].e.fallback.ably-realtime.com@. | |||
| *** @(REC2c5)@ Otherwise, if the @primary domain@ is determined to be a routing policy name via @(REC1c2)@ then the set of @fallback domains@ is @[name].a.fallback.ably-realtime.com@, @[name].b.fallback.ably-realtime.com@, @[name].c.fallback.ably-realtime.com@, @[name].d.fallback.ably-realtime.com@, @[name].e.fallback.ably-realtime.com@. | |||
| *** @(REC2c6)@ Otherwise, if the @primary domain@ is determined by the deprecated @restHost@ or @realtimeHost@ option via @(REC1d)@ then the set of fallback domains is empty. | |||
| *** @(REC2c5)@ Otherwise, if the routing policy name via @(REC1b5)@ is @sandbox@ then the set of @fallback domains@ is @sandbox.a.fallback.ably-realtime-nonprod.com@, @sandbox.b.fallback.ably-realtime-nonprod.com@, @sandbox.c.fallback.ably-realtime-nonprod.com@, @sandbox.d.fallback.ably-realtime-nonprod.com@, @sandbox.e.fallback.ably-realtime-nonprod.com@. | |||
There was a problem hiding this comment.
Same goes for REC2c5, it's exact interpretation of REC2c3, where if we set endpoint to nonprod:sandbox, we will get same behaviour. I don't think we need different spec for sandbox specific endpoint
There was a problem hiding this comment.
I believe specifying endpoint as nonprod:sandbox across all SDKs should work well, given that endpoint already involves its sub-clauses with given complexities. Introducing extra spec points for sandbox ( only used for testing ) would only add unnecessary complexity to the implementation across all SDKs.
Ideally, spec points should focus on aspects directly related to the implementation of business logic in the code. They shouldn’t address test-specific details, as they don’t contribute to the business logic itself. In general, even during normal development, we avoid mixing test-specific implementations with the actual business logic.
I also feel new spec violates REC1b4, as it introduces the sandbox keyword as an exception to the spec point.
|
Okay, endpoint ADR conclusion specifies,
This is not a breaking change, since this will be only for tests. Also, it says
That means specifying |
I actually missed this in the ADR but I really don't think we should do this, since implementing this on the infrastructure side will be complicated, since we have to consider SSL certificates etc, which reside in a different AWS account, and I would prefer not to do have to do that since it is significantly easier to build in backwards compatibility to the client itself, since it's a simple conditional. However, I am happy to remove the compatibility entirely and we can just agree that all tests are updated to use |
|
Yeah, thanks! |
In the initial implementation I wrote for ably-go I included backward compatibility to allow being able to simply specifying
sandboxrather than having to specifynonprod:sandbox, and to ensure this compatibility is included in all client implementations, then I am including it in the spec.Happy for suggestions on where it should go and wording, it seemed appropriate as a sub-clause.