tickets/DM-46480: adopt shared rubin-nublado-client

[athornton]

Rather than use internal client, use the shared client from the nublado repo.

This is currently dependent on DM-46480 branch of nublado, and will be changed once that work is merged and released.

[fajpunk]

My main architectural questions are:

• Would it be cleaner to keep a nublado support in mobu, whose only job is to wrap the extracted nublado client's methods and do the exception transormations, instead of doing those at each call site? Would that even be possible?
• Is there any way to extract out the common stuff we're doing in all of the wrapped exceptions, like adding annotations and mobu fields, somehow so we don't have to repeat the same logic in every wrapped exception?

[fajpunk]

If this isn't used in any calling code, can it just be smooshed into from_exception?

[athornton]

IDK. In most cases, from_exception() is how the base class turns its parent exception into the NubladoClientException. We'd be changing the call signature if we made it an override.

My tendency is to pick a different name, but maybe I'm wrong about that? I feel like it's a kind of useful distinction between "this is already a NubladoClientSlackException" (or SlackWeb) and therefore mostly-tame already, versus "naw, dawg, this is turning anything into one of our own exceptions".

[fajpunk]

Why do we lose the ability to test specific descriptions and references here?

[fajpunk]

(and elsewhere in these tests)

[athornton]

Because it wasn't clear to me that we would want to set them directly or use the ones from the NubladoClient testing suite, and there they are not exposed well. I think.

[athornton]

The thing here is, they're a magic constant (like "w_2077_43") that doesn't appear in any of the mobu code, because it's just being pulled in from the client testing code, and if the client testing code wants to change them (although, why would it?) then why should mobu break.

Maybe I should be exposing it in the client testing layer? But that feels weird too because it's basically just a test fixture.

[athornton]

The right way to address the architectural concerns (and this is something I talked about a little with @rra ) is ultimately to turn almost all of these exception fields into annotations. That will weaken the typing a little, but will make composing exceptions a whole lot easier.

It's a little subtle, in that we want to maintain the one-column vs two-column distinctions (that is, blocks versus fields) and large amounts of data will need to be packaged as attachments, and that (or something near there) is not documented in the Slack API...and then all of that really needs to be in Safir, neither in Mobu nor in NubladoClient.

But that's going to be a lot of work, so what I want to do in the immediate term is to land splitting out the NubladoClient into the Nublado monorepo, and adopting it into ghostwriter, mobu, and noteburst.

In addition to being a lot of work, the annotation-based Safir rewrite is work we should not undertake without Jonathan, who will have strong opinions about how Safir ought to work, so we really can't tackle it until 2025 when he's back.

[sqreadmin]

My main architectural questions are:

* Would it be cleaner to keep a nublado support in mobu, whose only job is to wrap the extracted nublado client's methods and do the exception transormations, instead of doing those at each call site?  Would that even be possible?

* Is there any way to extract out the common stuff we're doing in all of the wrapped exceptions, like adding annotations and mobu fields, somehow so we don't have to repeat the same logic in every wrapped exception?

Uh. Trying to think through this. Because the stuff we're adding isn't part of the client...buuuut if it ALSO took a CodeContext on each call, or updated a CodeContext object that DID live on the client....anyway, let's see.

[sqreadmin]

I guess it's the moral equivalent of _convert_exception() in the Nublado Client. Hm.

I still don't see how we would get away from updating each exception individually, though, because monkey and event depend on something that definitely isn't a client property.

There probably is some much more clever way to do it, but I think I'm going to put that off until we do the proper refactoring of the Safir base Slack exception class.

[sqreadmin]

Waaaaait why am I logged in as sqreadmin?

[fajpunk]

One more typo, but yeah, I guess we can put off a refactor until we do the bigger refactor