> I don’t think what what being proposed by Daniel was to include both short > and long names in the UI fields, but just in the get_conn method, right?
Hmm. That **might** work indeed. It will not be too helpful in having both forms possible and making long form "default" for UI, "short" default for other methods. It might add more confusion for the users - because right now there is "one" blessed way for each connection (even if the other way works, it is deprecated). Now we are trading it with no "different in different connections" variance, with "where connection is defined" variance. No strong opinions on that one - it am fine with that if others see no problem with this approach. It does not solve the root cause though so it is another band-aid over band-aid though :). J. On Tue, Nov 23, 2021 at 12:12 AM Alex Begg <[email protected]> wrote: > > Hi. I was actually the person who started this discussion on this PR: > https://github.com/apache/airflow/pull/19530 > > I understand the reasoning for the long names now, and I wanted to add to > this discussion that if you make a hook's get_conn method allow for both > short and long names, there isn't a necessity to alter the connection's extra > names for the UI, right? So if the connection needs the long extra name for > the UI customizations, you can replace every instance in the get_conn method > where it access that extra name (e.g. extra__google__hello) with > `extra.get('hello') or extra.get('extra__google__hello')` , and the UI > customizations will still work and the people managing connections via > secrets backends can use the shorter extra name. > > I don’t think what what being proposed by Daniel was to include both short > and long names in the UI fields, but just in the get_conn method, right? > > The only time I find the long extra names not desirable is when you are > defining connection values via URI (even as simple as via environment > variables) and you have to include the long names in the URI. > > In my opinion all of our connections should allow for short names if you are > not using the UI, since you can’t edit the connections via UI anyways. > > Alex > > > On Mon, Nov 22, 2021 at 8:01 AM Jarek Potiuk <[email protected]> wrote: >> >> Yeah. We really need to clean it up - but I am afraid without fixing >> the UI for managing the connections in a sane way, the status quo >> (even if it's not very clear how it works) will remain. >> >> J. >> >> On Mon, Nov 22, 2021 at 6:00 AM Josh Fell >> <[email protected]> wrote: >> > >> > To add a little wrinkle and small clarification here, there are some >> > connection types that expose and use both classic Extra as well as >> > customized fields in the connection form (Snowflake being the main example >> > I can think of). As of 2.2, these two Extra options are being merged but >> > not necessarily as a "smart" merge. If both are provided, the Extra JSON >> > simply combines the classic Extra and custom field values. >> > >> > On Sun, Nov 21, 2021 at 1:30 PM Jarek Potiuk <[email protected]> wrote: >> >> >> >> Also - why the MUST: >> >> >> >> If you use custom fields, in the current implementation, when you have >> >> both short and long form "available", when you edit such a connection, >> >> the "extras" dictionary is overridden with long names (simply because >> >> the JSON extra field is disabled and there is no "merge" logic for >> >> handling both. >> >> >> >> So there is a scenario where you have a "short" name defined in extra >> >> initially - but when you edit the connection, it will disappear >> >> (unless you manually fill the "form" field with the same value). >> >> >> >> That's why for connections that implement "custom" field behaviours >> >> the "short" form is really "deprecated" value (to support a case where >> >> the user uses connection_uri or has a database configured with "short" >> >> names from earlier manual "editing" the JSON field). >> >> >> >> And yes - it's not ideal, but I think the only proper way to handle it >> >> is to implement better behaviour of the UI and support for >> >> "namespaced" per-connection short names (which should not be too >> >> difficult for anyone with some flair for javascript + Flask App >> >> Builder). >> >> Until then, we have to live with the dichotomy, I am afraid. >> >> >> >> J. >> >> >> >> On Sun, Nov 21, 2021 at 7:18 PM Jarek Potiuk <[email protected]> wrote: >> >> > >> >> > The problem is that if we use short extras, until we fix that in the >> >> > UI, customizations won't work or they might override each other for >> >> > different connections. >> >> > >> >> > So my take on it is - (until we fix the UI): >> >> > >> >> > a) if the connection customizes UI fields (so that you DO NOT edit the >> >> > extras as JSON in the UI "extras" field manually) - you MUST use the >> >> > long form. >> >> > b) If the connection does not customize UI fields (so that you have to >> >> > edit extras as JSIN in the UI "extras" field manually) - it is >> >> > preferred to use short form >> >> > >> >> > J. >> >> > >> >> > On Sun, Nov 21, 2021 at 7:09 PM Daniel Standish <[email protected]> >> >> > wrote: >> >> > > >> >> > > I wasn't exactly proposing updating the front end to handle short >> >> > > names. It sounds like a reasonable idea but I am not trying to take >> >> > > that on right now. >> >> > > >> >> > > What I was trying to ask was, given the present form behavior, what >> >> > > should the hook behavior be, by convention? >> >> > > >> >> > > Specifically, should we write our conn parsing in hooks such that >> >> > > whenever there is a form extra (e.g. extra__google__hello) we always >> >> > > do `extra.get('hello') or extra.get('extra__google__hello')` >> >> > > >> >> > > Or something along those lines. >> >> > > >> >> > > Or should we stick with the status quo, which is to say there is no >> >> > > such convention and some hooks will accept both and others will not. >> >> > > >> >> > > I think my vote would be that by convention we should check both and >> >> > > short beats long in precedence. This doesn't mean we have to go >> >> > > update every hook with form customizations. But if we're reviewing a >> >> > > PR that checks both then it's ok to approve, and if we're reviewing a >> >> > > PR that only checks long name we can suggest checking short name >> >> > > also. >> >> > > >> >> > > WDYT? >> >> > > >> >> > >
