Amogh:
> Should we enforce creating a follow up Github issue in our case for the
same? This would solve the issue of unfinished PRs
while also giving confidence that the comments won't be ignored.
I think we already do that in a number of cases. And I would not enforce
it, especially that (unlike
I generally like the idea of adding this rule but I have a few concerns
around the amount of unfinished/incomplete PRs this could lead to.
We generally follow such things in my org where we try to resolve all the
comments from the maintainers
before performing a merge. But what we also do is that
Dennis:
> I guess as long as we are not dictating that the person who left the
comment has to resolve it, then I'm alright with trying this. I don't want
a PR to get blocked because of a drive-by comment.
Yep. I think it is perfectly fine for the person who reviews and wants to
merge the code t
Interesting. I generally try to follow that policy as a best practice on my
own PRs just so I make sure I didn't miss comments, but there are also times I
intentionally leave certain discussions "out in the open". I guess as long as
we are not dictating that the person who left the comment h
> Given the slow down over the holidays I don't think two weeks will be
enough - make it 4?
Ah. True :)
On Tue, Dec 19, 2023 at 9:41 PM Ash Berlin-Taylor wrote:
> Given the slow down over the holidays I don't think two weeks will be
> enough - make it 4?
>
> On 19 December 2023 20:33:23 GMT, Ja
Given the slow down over the holidays I don't think two weeks will be enough -
make it 4?
On 19 December 2023 20:33:23 GMT, Jarek Potiuk wrote:
>Ash:
>
>> I.e. Convention over enforcement and treating people as mature adults not
>children who need guard rails.
>
>I think it's quite the opposite,
Ash:
> I.e. Convention over enforcement and treating people as mature adults not
children who need guard rails.
I think it's quite the opposite, Both 1) 2) and 3) reasoning is more of the
aid for whoever looks at the PR that there are still some conversations not
addressed, I personally feel it's
Answering some of the recent questions.
Daniel:
> E.g. when you "resolve" a conversation, then you make it less visible.
This isn't always a good thing. Sometimes you just want to +1 it. When
others visit the PR, then they will not see the conversation. Maybe they
would want to engage in discu
This reflect my feelings as well. I'm not convinced we are solving something
that needs to be solved.
B.
Sent from my iPhone
> On 19 Dec 2023, at 21:05, Ash Berlin-Taylor wrote:
>
> Weak -1 from me, because I don't think this needs to be enforced/required
> part of the workflow.
>
> I.e. C
Weak -1 from me, because I don't think this needs to be enforced/required part
of the workflow.
I.e. Convention over enforcement and treating people as mature adults not
children who need guard rails.
-ash
On 19 December 2023 13:12:05 GMT, Jarek Potiuk wrote:
>Hey everyone,
>
>TL;DR; I have a
The proposed rule isn't a bad idea, especially for ensuring that
maintainers wanting to merge have reviewed all conversions. However, it's
essential to permit them to close open conversations if they find the
comments have been addressed. Only ping the commenter if uncertain, with a
maximum waiting
Well let me add some more thoughts.
I like the idea in general, the principle of trying to somehow acknowledge
the comments and suggestions that have been made.
But it may have some unintended and perhaps undesirable consequences.
E.g. when you "resolve" a conversation, then you make it less vis
+1
On Tue, Dec 19, 2023 at 9:36 AM Pierre Jeambrun
wrote:
> This is something I already try to apply on my own PRs, never merge before
> explicitly solving all conversations.
>
> Also for a reviewer, I feel like this gives more confidence to the fact
> that the PR is ready, and indeed we are les
This is something I already try to apply on my own PRs, never merge before
explicitly solving all conversations.
Also for a reviewer, I feel like this gives more confidence to the fact
that the PR is ready, and indeed we are less subject to missing a
discussion or something going on making it 'not
1+ enabling this check.
We use it for our company Github pipelines and for me this makes a clear
explicit check that nothing is "forgotten" before merge.
Still depending on responsiveness of reviewers this could block things if
answers are missing. Therefore I assume every committer can resolve
Great questions. More context then, since you asked:
The main benefits I see:
1) the person merging the PR will not merge PRs when conversations are
still unresolved manually. Some of the PRS have long conversations and
I had many times - for example - cases where I missed that someone
made a com
I am wondering too if this is not something that gives more work to maintainer
without real benefits. A maintainer can still mark all conversations as
resolved and merge the PR if he wants. Though, I understand there is the
intention here as oppose as today where a maintainer can just miss some
I’d vote for 2-a. Removing it in the upcoming minor release may not give enough
time for airflow users to adjust. On the other hand, we would also want to
decrease the amount of time we spend supporting various versions of Pendulum.
2.9 might be a reasonable compromise.
Best,
Wei
> On Dec 19,
I'm less enthusiastic. What problem are we solving with this? If something has
not been addressed it can be done in a follow up or of if it was just part of
the conversation it won't have impact on the code. In addition, the ones that
need to deal with it are the ones merging and those are not n
+1 for trying and observing how it works. My concern is that adding an
additional obstacle might lead to more unfinished PRs. It might be helpful to
give the contributor some guidance on when we can resolve the comments.
Best,
Wei
> On Dec 19, 2023, at 9:28 PM, Andrey Anshin wrote:
>
> We cou
+1 (non-binding)
Best,
Wei
> On Dec 19, 2023, at 8:20 PM, utkarsh sharma wrote:
>
> +1 non binding
>
> On Tue, 19 Dec 2023 at 5:23 PM, Amogh Desai
> wrote:
>
>> +1 non binding
>>
>> Installed the RC bits and ran example DAGs.
>>
>>
>>
>> On Tue, 19 Dec 2023 at 5:22 PM, Phani Kumar
>> wr
I am for a) with deprecation warning when Pendulum 2 is installed in
Airflow 2.8.1+.
I understand we have a long way before we can get rid of Pendulum
dependency in general as it will be a major pain for our users for a
number of reasons, so I think getting rid of Pendulum is likely not
easy / fea
Greetings everyone!
Because pendulum 3 was released we probably need to discuss about support
for the previous version of pendulum in Airflow core.
There is not much options here
Option 1: Drop support of next patch version of Airflow, e.g. 2.8.1. It
might (or might not) break someone's pipeline
We could try and if found it slows down for some reason we always might
revert it back.
Just one suggestion, sometimes discussion contains some useful information,
e.g. "What the reason of finally decision", "Useful information why it
should works by suggested way, or should not work", which might
I like the idea, at least double checking what the comments were is
a good cause. +1 to incorporating this.
On 2023/12/19 13:12:05 Jarek Potiuk wrote:
> Hey everyone,
>
> TL;DR; I have a small proposal/discussion proposal to modify a bit the
> branch protection rules for Airflow. Why don't we
Hey everyone,
TL;DR; I have a small proposal/discussion proposal to modify a bit the
branch protection rules for Airflow. Why don't we add a protection
rule in our PRs that requires all the comments in the PRs to be
"marked as resolved" before merging the PR ?
I have been following myself - for
+1 non binding
On Tue, 19 Dec 2023 at 5:23 PM, Amogh Desai
wrote:
> +1 non binding
>
> Installed the RC bits and ran example DAGs.
>
>
>
> On Tue, 19 Dec 2023 at 5:22 PM, Phani Kumar
> wrote:
>
> > +1 non binding
> >
> > On Tue, Dec 19, 2023 at 5:10 PM Pankaj Koti
> > wrote:
> >
> > > +1 (non-
+1 non binding
Installed the RC bits and ran example DAGs.
On Tue, 19 Dec 2023 at 5:22 PM, Phani Kumar
wrote:
> +1 non binding
>
> On Tue, Dec 19, 2023 at 5:10 PM Pankaj Koti
> wrote:
>
> > +1 (non-binding)
> >
> > Installed the RC and was able to trigger a DAG successfully using the
> > cli
+1 non binding
On Tue, Dec 19, 2023 at 5:10 PM Pankaj Koti
wrote:
> +1 (non-binding)
>
> Installed the RC and was able to trigger a DAG successfully using the
> client.
>
> Best regards,
>
> *Pankaj Koti*
> Senior Software Engineer (Airflow OSS Engineering team)
> Location: Pune, Maharashtra, In
+1 (non-binding)
Installed the RC and was able to trigger a DAG successfully using the
client.
Best regards,
*Pankaj Koti*
Senior Software Engineer (Airflow OSS Engineering team)
Location: Pune, Maharashtra, India
Timezone: Indian Standard Time (IST)
Phone: +91 9730079985
On Tue, Dec 19, 2023
Hey fellow Airflowers,
I have cut the release candidate for the Airflow Python Client 2.8.0rc1.
The client consists of APIs corresponding to REST APIs available in
*Apache Airflow 2.8.0*. This email is calling for a vote on
the release, which will last for 72 hours. Consider this my (binding) +1.
31 matches
Mail list logo
