+1, stable API should be favoured
On Tue 20 Feb 2024 at 16:10, Jarek Potiuk <ja...@potiuk.com> wrote:
> Oh God . What an archeology. Yeah I think we had a limite when we discussed
> it then (looking at the discussion) we did not have a solid replacement yet
> (and the discussion was about deprecating it in 1.10). Since then we marked
> it as experimental and left it in 2.0.
> But since then - we have - and it's well established and proven (REST
> API + Python Client) which very much serves the same purpose that the
> "remote" experimental CLI should serve (and I think that was the main
> problem that it did not exist back then).
>
> I'd say it calls for removal and significant note ("If you were still using
> it, please use the REST API and Python Client instead").
>
> On Tue, Feb 20, 2024 at 11:19 AM Andrey Anshin <andrey.ans...@taragol.is>
> wrote:
>
> > BTW, I've found that this this top already discussed in the past, like
> more
> > that 3 years ago
> >
> > Github Issue https://github.com/apache/airflow/issues/10552 (accidently
> > found when set sorting on issues as Least recently updated)
> > Dev List:
> https://lists.apache.org/thread/jdz9l7bsnsw5c3t27dxfrx5pd4wvjlxt
> >
> >
> >
> > On Tue, 20 Feb 2024 at 00:14, Andrey Anshin <andrey.ans...@taragol.is>
> > wrote:
> >
> > > Just for clarification, the proposal is about depreciation or even
> > removal
> > > of something obsolete. If we could do some improvement at the same
> moment
> > > it would be nice, if not also nice, that mean no additional work
> > >
> > >
> > >
> > >
> > >
> > > On Mon, 19 Feb 2024 at 22:55, Scheffler Jens (XC-AS/EAE-ADA-T)
> > > <jens.scheff...@de.bosch.com.invalid> wrote:
> > >
> > >> I do not habe a strong opinion on the topic but I am mostly with
> Jarek.
> > >>
> > >> The API must be multi tenant and mask all internal DB access - where
> we
> > >> are close-by with AIP-44 and the other efforts of multi tenancy
> > (missing a
> > >> list of all planned efforts, would be cool to have a mata AIP keeping
> > the
> > >> completed and planned individual efforts)
> > >>
> > >> But in contrast I see the CLI only partly being a tool on tenant but
> > most
> > >> parts on the internal perimeter admin side. Might be a bit „gray
> zone“.
> > If
> > >> we habe overlap to REST API (do we have a list of rhings like trigger
> > sag
> > >> or so?) we should remove and consolidate redundancy to API logic. But
> > there
> > >> is for sure some „black magic“ which must not be migrated and needs
> > native
> > >> DB access. E.g. resetting DB or triggering DB migration. For REST API
> as
> > >> well as internal API you need a web endpoint incl. minimal
> > authentication.
> > >> Some base admin tooling is needed for bootstrap and base admin. Like
> > >> creating the first admin user is impossible if no user.
> > >>
> > >> So before a lengthy discussion I‘d propose to document details and
> make
> > >> an AIP to discuss, that might be easier then a reply-to-all thred to
> > define
> > >> the scope of cleanup. Also and especially for API cöient we need to
> > check
> > >> for impact if breaking changes appear.
> > >>
> > >> Jens
> > >>
> > >> Sent from Outlook for iOS<https://aka.ms/o0ukef>
> > >> ________________________________
> > >> From: Andrey Anshin <andrey.ans...@taragol.is>
> > >> Sent: Monday, February 19, 2024 3:54 PM
> > >> To: dev@airflow.apache.org <dev@airflow.apache.org>
> > >> Subject: Re: [DISCUSS] Deprecate cli options in Airflow Configurations
> > >> and airflow.api.client
> > >>
> > >> >Actually - it's not at all part of AIP-44 :). Airflow CLI was
> > >> (consciously and deliberately) not part of it.
> > >>
> > >> Then better to check whether or not it accidentally uses it, add to my
> > >> checklist
> > >>
> > >> > So I would be very much for removing that whole part - even in
> Airflow
> > >> 2.9.
> > >>
> > >> +1 for removal if it won't break our promises
> > >>
> > >>
> > >>
> > >>
> > >> On Mon, 19 Feb 2024 at 16:01, Jarek Potiuk <ja...@potiuk.com> wrote:
> > >>
> > >> > Actually - it's not at all part of AIP-44 :). Airflow CLI was
> > >> (consciously
> > >> > and deliberately) not part of it.
> > >> >
> > >> > CLI is specifically treated as a "local" tool that is executed
> inside
> > >> the
> > >> > security perimeter where direct database access is needed (i.e. it
> > >> falls in
> > >> > the same category as 'scheduler', 'webserver`, `internal-api`
> > component
> > >> -
> > >> > all of which stil has direct DB access and AIP-44 does not change
> > that.
> > >> > AIP-44 is specifically about Worker, Triggerrer and Dag File
> Processor
> > >> > direct DB access only.
> > >> >
> > >> > I think if we want to allow CLI to be used without direct DB
> access, a
> > >> > better solution would be to make it use the public REST API of ours,
> > not
> > >> > the internal api that is supposed to be used by worker/triggerer/dag
> > >> file
> > >> > processor. There will be different authentication and authorisation
> > >> methods
> > >> > than the ones that airflow CLI would - potentially use. I think if
> we
> > >> want
> > >> > to make airflow CLI use authorised/remote access - that needs a
> > separate
> > >> > AIP - where we define actors that should be able to use the CLI,
> > >> > authorisation mechanisms etc. (could be the same as in the REST API,
> > >> but it
> > >> > needs to be clarified - currently the authorisation of CLI is based
> > >> > exclusively on having access to the system/UNIX user that airflow is
> > run
> > >> > with and where DB configuration to access DB is present).
> > >> >
> > >> > But coming back to the main subject - yes, I think that old API is
> > >> > experimental, and for a long time we have good replacement for it
> > (i.e.
> > >> the
> > >> > official REST API that has been battle-tested and is pretty
> > >> feature-full),
> > >> > so It is a good idea to remove the old API (and CLI configuration
> > >> option).
> > >> > It's not likely widely used, and it's also had experimental status
> > for a
> > >> > very long time (since the beginning of Airflow 2.0) - so similarly
> as
> > >> with
> > >> > the MSSQL support, we are free to decide to remove it and still keep
> > our
> > >> > SemVer promises - it never made it on the Public Interface of
> Airflow
> > >> >
> > >> >
> > >>
> >
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fairflow.apache.org%2Fdocs%2Fapache-airflow%2Fstable%2Fpublic-airflow-interface.html&data=05%7C02%7CJens.Scheffler%40de.bosch.com%7C7183c980c1634387b9c808dc315ab031%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638439512771163568%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=hJSW%2FH7qQHQIH0YFHjxAoHWBNSejl2yUBpMx1Q9PQAo%3D&reserved=0
> > >> <
> > >>
> >
> https://airflow.apache.org/docs/apache-airflow/stable/public-airflow-interface.html
> > >> >
> > >> > - and since the very beginning we did not have an intention for it
> to
> > be
> > >> > public API.
> > >> >
> > >> > So I would be very much for removing that whole part - even in
> Airflow
> > >> 2.9.
> > >> >
> > >> > J.
> > >> >
> > >> > On Mon, Feb 19, 2024 at 11:53 AM Andrey Anshin <
> > >> andrey.ans...@taragol.is>
> > >> > wrote:
> > >> >
> > >> > > > the underlying question is what are we going to do with direct
> DB
> > >> > access
> > >> > > from the cli
> > >> > >
> > >> > > Good point!
> > >> > > I guess it should be covered by AIP-44, so if there are no methods
> > >> which
> > >> > > implement these actions by AIP-44 it is better to create it
> instead
> > of
> > >> > just
> > >> > > moving old code.
> > >> > >
> > >> > >
> > >> > >
> > >> > > On Mon, 19 Feb 2024 at 14:35, Bolke de Bruin <bdbr...@gmail.com>
> > >> wrote:
> > >> > >
> > >> > > > with regards to the api client: the intention was indeed to
> remove
> > >> > direct
> > >> > > > access to the DB, which I think is still a valid thing to do. In
> > my
> > >> > > opinion
> > >> > > > the underlying question is what are we going to do with direct
> DB
> > >> > access
> > >> > > > from the cli. Is that something we want to keep or do we want a
> > >> > different
> > >> > > > design? Just dropping the implementation seems a bit 'lazy' to
> me,
> > >> > while
> > >> > > I
> > >> > > > understand that it hasn't been touched in a while.
> > >> > > >
> > >> > > > B.
> > >> > > >
> > >> > > > On Sun, 18 Feb 2024 at 12:17, Andrey Anshin <
> > >> andrey.ans...@taragol.is>
> > >> > > > wrote:
> > >> > > >
> > >> > > > > Greetings guys!
> > >> > > > >
> > >> > > > > I want to start a discussion about deprecation cli
> configuration
> > >> > > options
> > >> > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> >
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fairflow.apache.org%2Fdocs%2Fapache-airflow%2Fstable%2Fconfigurations-ref.html%23cli&data=05%7C02%7CJens.Scheffler%40de.bosch.com%7C7183c980c1634387b9c808dc315ab031%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638439512771174117%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=dQ3MPz8rgbWo%2FIezHpvAuJ4fxDAe%2FT9hx3WRloR3ZRQ%3D&reserved=0
> > >> <
> > >>
> >
> https://airflow.apache.org/docs/apache-airflow/stable/configurations-ref.html#cli
> > >> >
> > >> > > > > as well as `airflow/api/client` in upcoming minor/feature
> > release
> > >> of
> > >> > > > > Airflow (2.9 or 2.10 depends on).
> > >> > > > >
> > >> > > > > This options control how end users retrieve information from
> the
> > >> > > > Database,
> > >> > > > > there is two kind provided classes
> > >> > > > `airflow.api.client.api_client.Client` -
> > >> > > > > direct access to DB, `airflow.api.client.json_client.Client` -
> > >> access
> > >> > > > > through Experimental API
> > >> > > > >
> > >> > > > > However internal clients cover only this cli commands:
> > >> > > > > - trigger_dag: airflow dags trigger
> > >> > > > > - delete_dag: airflow dags delete
> > >> > > > > - get_pool: airflow pool get
> > >> > > > > - get_pools: airflow pool list and airflow pool export
> > >> > > > > - create_pool: airflow pool set and airflow pool import
> > >> > > > > - pool_delete: airflow pool delete
> > >> > > > > - get_lineage: doesn't use anywhere in CLI and Airflow
> codebase
> > >> > > > >
> > >> > > > > As a result, implementation of CLI spread across different
> > >> modules /
> > >> > > > > subpackages and in some cases could use experimental API.
> Seems
> > >> like
> > >> > it
> > >> > > > is
> > >> > > > > an abandoned implementation which comes from pre Airflow 2 and
> > we
> > >> > have
> > >> > > to
> > >> > > > > support it nowadays.
> > >> > > > >
> > >> > > > > *My proposal*:
> > >> > > > > 1. Copy implementation from
> > >> `airflow.api.client.local_client.Client`
> > >> > > into
> > >> > > > > the the code from appropriate modules into the
> > >> airflow/cli/commands
> > >> > > > > 2. Set default value for `[cli] api_client` to None. In case
> if
> > >> > values
> > >> > > > set
> > >> > > > > in airflow.cfg (e.g. from the previous version of Airflow),
> > raise
> > >> > > future
> > >> > > > > warning and update it to None if it set to
> > >> > > > > `airflow.api.client.local_client.Client` in other cases do not
> > >> touch
> > >> > > > > 3. Set default value for `[cli] endpoint_url` to None. In case
> > if
> > >> > > values
> > >> > > > > set in airflow.cfg raise future warning, and do not update
> > value.
> > >> > > > > 4. `airflow.api.client.get_current_api_client` should able to
> > >> return
> > >> > > > None,
> > >> > > > > if it return None, then it should use implementation from the
> > >> > > > > airflow/cli/commands, otherwise use deprecated client, with
> > >> raising
> > >> > > > > RemovedInAirflow3Warning
> > >> > > > >
> > >> > > > > WDYT?
> > >> > > > >
> > >> > > >
> > >> > > >
> > >> > > > --
> > >> > > >
> > >> > > > --
> > >> > > > Bolke de Bruin
> > >> > > > bdbr...@gmail.com
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>
