-
Notifications
You must be signed in to change notification settings - Fork 17
Use endpoint as default connection option (ADR-119) #441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Rubocop needs to be configured for this repository, but is currently quite a bit of work to get it up to standards[1]. While we get to actually enabling Rubocop, it would be nice to be explicit that it's not something that my editor should bother me with when working in this repository. [1] #139
d3e56fb
to
cc7ab95
Compare
cc7ab95
to
7d81b61
Compare
7d81b61
to
bae5bc3
Compare
bae5bc3
to
4f76a08
Compare
v3 is deprecated.
8578bee
to
491a6a6
Compare
491a6a6
to
1cda434
Compare
617e8f7
to
72ccd71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @surminus! A few thoughts from an initial review:
- do you think you could split out the rename of the
endpoint
method into a separate, preceding commit? it would help with readability if the functional changes and the refactor were separate - please could you add comments to mention the spec points (see Updates for ADR-119: updated ClientOptions for new domains specification#213) that the new code relates to?
- please could you add tests for the new functionality (i.e. for the
endpoint
client option), again with reference to the relevant spec points - where spec points have been removed or replaced (e.g. RTN17b, a bunch under RSC15) please could you update the corresponding code and tests that make reference to these spec points?
@lawrence-forooghian thanks for the review, these all seem like good points, I will try to get to them in the next week or two! |
The `endpoint` naming is going to be used as a client option, so we need to rename everywhere this is currently used to avoid conflation.
This implements ADR-119[1], which specifies the client connection options to update requests to the endpoints implemented as part of ADR-042[2]. The endpoint may be one of the following: * a routing policy name (such as `main`) * a nonprod routing policy name (such as `nonprod:sandbox`) * a FQDN such as `foo.example.com` The endpoint option is not valid with any of environment, restHost or realtimeHost, but we still intend to support the legacy options. If the client has been configured to use any of these legacy options, then they should continue to work in the same way, using the same primary and fallback hostnames. If the client has not been explicitly configured, then the hostnames will change to the new `ably.net` domain when the package is upgraded. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3428810778/ADR-119+ClientOptions+for+new+DNS+structure [2] https://ably.atlassian.net/wiki/spaces/ENG/pages/1791754276/ADR-042+DNS+Restructure
The expected result has changed in this test, as per this internal Slack thread[1]. [1] https://ably-real-time.slack.com/archives/CURL4U2FP/p1733479167314059?thread_ts=1733420816.469159&cid=CURL4U2FP
72ccd71
to
f8eba18
Compare
@lawrence-forooghian I've broken out the commit for renaming the RE: tests, even though in my initial comment I said I hadn't added tests, I do seem to have added a new context for using endpoint in the client initializer[1], is this sufficient? Or do we want to include new contexts everywhere we use an I'm going to look at updating the spec references now. [1] ably-ruby/spec/shared/client_initializer_behaviour.rb Lines 269 to 312 in f8eba18
|
@lawrence-forooghian I've updated the specs where they seem relevant, but there weren't actually that many references? It seemed a little inconsistent in the way they were applied. Do I need to also update |
@surminus as discussed on Slack, please could you make sure that this PR implements / tests the updates to REC1b2 from ably/specification#302? Thanks! |
a54886c
to
6fa7260
Compare
6fa7260
to
c5a9325
Compare
@lawrence-forooghian I've updated the tests and added appropriate annotations |
This implements ADR-119[1], which specifies the client connection options to update requests to the endpoints implemented as part of ADR-042[2].
The endpoint may be one of the following:
main
)nonprod:sandbox
)foo.example.com
The endpoint option is not valid with any of environment, restHost or realtimeHost, but we still intend to support the legacy options.
If the client has been configured to use any of these legacy options, then they should continue to work in the same way, using the same primary and fallback hostnames.
If the client has not been explicitly configured, then the hostnames will change to the new
ably.net
domain when the package is upgraded.Notes on implementation
This is still a work in progress, I'm opening a PR to allow tests to run.
I've tried to change as little as possible, but we already had a method named
endpoint
so I've switched that touri
(open to better naming suggestions).I haven't yet added any explicit tests for using
endpoint
, and tried to limit changes to the existing to the updated hostname.[1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3428810778/ADR-119+ClientOptions+for+new+DNS+structure
[2] https://ably.atlassian.net/wiki/spaces/ENG/pages/1791754276/ADR-042+DNS+Restructure