t's easier to get to.
From: Bruce Schuchardt
Sent: Tuesday, March 30, 2021 08:07
To: dev@geode.apache.org
Subject: request for people putting up pull requests
If you want people to review your PR it would be kind of you to put in a
description of its purpos
If you want people to review your PR it would be kind of you to put in a
description of its purpose. If you just reference a JIRA ticket we have to do
the extra work of pulling up the ticket to see what it’s about. I’m seeing
this more and more lately.
Hey All,
While it appears to be common practice for us to all pay attention to and burn
down pull requests on the primary apache/geode repository. It would be really
great if you all could take time to look at some of our other repositories and
help out with PRs. The apache/geode-native
I’m still not interested until there is a solution for all community members to
retrigger failed jobs. Also not excited about not having a way to override if
there is a known issue that prevents a job from going green. My last PR needed
multiple reruns because of a known flakey test with an acti
I’d like to follow up on this discussion from late last year. Six months ago,
Kirk wrote:
> After we get it more consistently GREEN, I would be willing to change my vote
> to +1.
While we’re still not perfect, I have noticed that PR checks go green much more
consistently now than they did six
I'm changing my vote to -1 for disallowing merge until precheck passes.
The reason is that it's rare for me to see a 100% clean precheckin for any
of my PRs. There seems to always be some failure unrelated to my PR. For
example PR #3042 just hit GEODE-6008. If we make the change to disable the
mer
I was responding to Udo's comment:
"Could one not configure the button that the owner of the PR cannot merge
the PR?"
I'm +1 for disallowing merge until precheck passes.
I'm -1 for disallowing the owner of the PR to merge it.
On Fri, Dec 21, 2018 at 9:28 AM Helena Bales wrote:
> Kirk, this cha
Kirk, this change would not require you to get someone to merge it. It
would just require that your PR pass CI before it can be merged.
On Thu, Dec 20, 2018 at 2:38 PM Kirk Lund wrote:
> I have enough trouble just getting other developers to review my PR. I
> don't want to have to struggle to fi
I have enough trouble just getting other developers to review my PR. I
don't want to have to struggle to find someone to merge it for me, too.
On Mon, Nov 19, 2018 at 4:09 PM Udo Kohlmeyer wrote:
> I don't believe "name and shame" is a hammer we should wield, but if we
> have use it... use it wi
+1 to blocking the "merge" button
On Mon, Nov 19, 2018 at 5:09 PM Udo Kohlmeyer wrote:
> I don't believe "name and shame" is a hammer we should wield, but if we
> have use it... use it wisely
>
> Could one not configure the button that the owner of the PR cannot merge
> the PR?
>
> --Udo
>
>
>
;>> On Fri, Nov 9, 2018 at 2:36 PM Kirk Lund wrote:
>>>>>>
>>>>>> Well, we could run it periodically such as weekly rather than as part of
>>>>>> the main pipeline or precheckin.
>>>>>>
>>>>>> On F
>>>>> On Fri, Nov 9, 2018 at 2:32 PM, Aditya Anchuri
>>>>> wrote:
>>>>>
>>>>>> +1, although I do wonder about the overhead of making PRs increasing more
>>>>>> than it already feels like to me as a new contributor (
2018 at 2:20 PM Bruce Schuchardt
wrote:
I'd like to see LGTM run on pull requests. Otherwise I think we're
fighting a losing battle trying to improve the quality of our code. For
instance, we just had a nice contribution of geospatial functionality
that raised 5 alerts, but we only fo
I don't believe "name and shame" is a hammer we should wield, but if we
have use it... use it wisely
Could one not configure the button that the owner of the PR cannot merge
the PR?
--Udo
On 11/19/18 16:03, Dan Smith wrote:
Closing the loop on this thread - I don't feel like there was enou
Closing the loop on this thread - I don't feel like there was enough
agreement to go forwards with disabling the merge button, so I'm going to
drop this for now.
I would like to see everyone make sure that they only merge green PRs.
Maybe we can go with a name and shame approach? As in, we shouldn
+1 I like this idea, but I recognize that it will be a challenge when there
is still some flakiness to the pipeline. I think we'd need clear
guidelines on what to do if your PR fails due to something seemingly
unrelated. For instance, we ran into GEODE-5943 (flaky EvictionDUnitTest)
in our last P
Just to clarify, that flaky EvictionDUnitTest is old flaky. The PR to
refactor the test passed all checks, even the repeatTest as well. I had a
closed PR that just touched the "un-refactored" EvictionDUnitTest, it
wouldn't even pass the repeatTest at all.
On Mon, Nov 12, 2018 at 2:04 PM Dan Smith
In that case, why don't we make the rule that the owner of the PR is not
allowed to merge it.
Another committer must then accept and merge it.
--Udo
On 11/12/18 14:03, Dan Smith wrote:
To be clear, I don't think we have an issue of untrustworthy committers
pushing code they know is broken to
To be clear, I don't think we have an issue of untrustworthy committers
pushing code they know is broken to develop.
The problem is that it is all to easy to look at a PR with some failures
and *assume* your code didn't cause the failures and merge it anyway. I
think we should all be at least reru
0
Patrick does make a point. The committers on the project have been voted
in as "responsible, trustworthy and best of breed" and rights and
privileges according to those beliefs have been bestowed.
We should live those words and trust our committers. In the end.. If
there is a "rotten" appl
-1
I really don't think this needs to be codified. If people are merging red
PRs, that is a failing as a responsible developer. Defensive programming
is all well and good, but this seems like it's a bit beyond the pale in
that regard. I foresee it making the correction of a red main pipeline
mu
I'm in favor of this change, but only if we have a way to restart failing
PR builds without being the PR submitter. Any committer should be able to
restart the build. The pipeline is still flaky enough and I want to avoid
the situation where a new contributor is asked repeatedly to push empty
commi
What's the general consensus on flakiness of the pipeline for this purpose?
If there is consensus that it's still too flaky to disable the merge button
on failure, we should probably consider doubling down on that issue again.
It's hard to tell from just looking at the dev pipeline because you cann
I'm in favor of this.
Several times over the years we've made a big push to get precheckin to
reliably only to see rapid degradation due to crappy code being pushed
in the face of precheckin failures. We've just invested another half
year doing it again. Are we going to do things differently
-1
I think there is still plenty of flakiness in the tests that PRs can go red
due to something flaky. Blocking a merge when the issue is known to be
flaky would be disruptive to progress. I think it will also just encourage
someone to use direct pushing to work around false reds. This might lead
+1
> On Nov 9, 2018, at 2:07 PM, Helena Bales wrote:
>
> +1
> As far as the failure rate goes, even if the rate is 25% restarting the
> pipeline usually gives a passing run. And this would make us less likely to
> incorrectly dismiss a failure as not related. If the second build also
> fails wit
ore
>>>> than it already feels like to me as a new contributor (as the person who
>>>> made the geospatial contribution). If this was a gradle task maybe like
>>>> spotless?
>>>>
>>>> On Fri, Nov 9, 2018 at 2:20 PM Bruce Schuchardt
>> than it already feels like to me as a new contributor (as the person who
>>> made the geospatial contribution). If this was a gradle task maybe like
>>> spotless?
>>>
>>> On Fri, Nov 9, 2018 at 2:20 PM Bruce Schuchardt
>>> wrote:
>>>
&
new contributor (as the person who
> > made the geospatial contribution). If this was a gradle task maybe like
> > spotless?
> >
> > On Fri, Nov 9, 2018 at 2:20 PM Bruce Schuchardt
> > wrote:
> >
> > > I'd like to see LGTM run on pull requests. Otherw
or (as the person who
> made the geospatial contribution). If this was a gradle task maybe like
> spotless?
>
> On Fri, Nov 9, 2018 at 2:20 PM Bruce Schuchardt
> wrote:
>
> > I'd like to see LGTM run on pull requests. Otherwise I think we're
> > fighting a
7;d like to see LGTM run on pull requests. Otherwise I think we're
> fighting a losing battle trying to improve the quality of our code. For
> instance, we just had a nice contribution of geospatial functionality
> that raised 5 alerts, but we only found out about it after the code w
u all
have made for LGTM. Thanks by the way!
On Fri, Nov 9, 2018 at 2:20 PM, Bruce Schuchardt
wrote:
> I'd like to see LGTM run on pull requests. Otherwise I think we're
> fighting a losing battle trying to improve the quality of our code. For
> instance, we just had a ni
I'd like to see LGTM run on pull requests. Otherwise I think we're
fighting a losing battle trying to improve the quality of our code. For
instance, we just had a nice contribution of geospatial functionality
that raised 5 alerts, but we only found out about it after the code was
+1
As far as the failure rate goes, even if the rate is 25% restarting the
pipeline usually gives a passing run. And this would make us less likely to
incorrectly dismiss a failure as not related. If the second build also
fails with the same failures, those should be investigated and resolved in
so
It looks like the current failure rate (post-PR, including all types of
failures) for DistributedTest is around 25%. Do most people experience similar
failure rates on the *-pr pipeline? I’m specifically wondering about failures
unrelated to your changes.
Anthony
> On Nov 9, 2018, at 12:55
+1
On Fri, Nov 9, 2018 at 12:58 PM Robert Houghton
wrote:
> +1
>
> On Fri, Nov 9, 2018, 12:55 Dan Smith
> > Hi all,
> >
> > Kirks emails reminded me - I think we are at the point now with our PR
> > checks where we should not be merging anything to develop that doesn't
> pass
> > all of the PR
+1 assuming “all of the PR checks” will soon include Java 11
Implementation-wise should be as simple at getting someone with admin
privileges on the geode repo to change the settings for “develop” branch to
"Require status checks to pass before merging."
> On Nov 9, 2018, at 12:58 PM, Robert Ho
+1
On Fri, Nov 9, 2018 at 12:58 PM, Robert Houghton
wrote:
> +1
>
> On Fri, Nov 9, 2018, 12:55 Dan Smith
> > Hi all,
> >
> > Kirks emails reminded me - I think we are at the point now with our PR
> > checks where we should not be merging anything to develop that doesn't
> pass
> > all of the PR
+1
On Fri, Nov 9, 2018, 12:55 Dan Smith Hi all,
>
> Kirks emails reminded me - I think we are at the point now with our PR
> checks where we should not be merging anything to develop that doesn't pass
> all of the PR checks.
>
> I propose we disable the merge button unless a PR is passing all of
Hi all,
Kirks emails reminded me - I think we are at the point now with our PR
checks where we should not be merging anything to develop that doesn't pass
all of the PR checks.
I propose we disable the merge button unless a PR is passing all of the
checks. If we are in agreement I'll follow up wi
Hello Bruce,
Sounds good to me, thanks for the quick reply!.
Best regards.
On Tue, May 22, 2018 at 4:44 PM Bruce Schuchardt
wrote:
> I don't think the "single commit" notion is even a good one for an
> original PR. I've discussed this with other people and we think it's
> okay for the PR to h
I don't think the "single commit" notion is even a good one for an
original PR. I've discussed this with other people and we think it's
okay for the PR to have multiple commits if they serve different
purposes such as renaming variables or restructuring code before
attacking a problem. Likewi
Hello geode devs,
The Geode Wiki has a lot of useful information, not only about the
usage/internal architecture of Geode, but also explanations and guidelines
about how Code Contributions [1] should be made. However, some common cases
are not explained in detail and, as such, contributors (and al
ly on the ASF Geode repositories.
> >
> > * Submit GitHub pull requests, following all current rules for pull
> > requests, for changes that would require peer review before committing to
> > the production branches.
> >
> > * Register their GitHub acc
committers such that committers shall:
>
> * Perform all work in progress on branches in their personal forks on
> GitHub rather than directly on the ASF Geode repositories.
>
> * Submit GitHub pull requests, following all current rules for pull
> requests, for changes that
Sent from my iPhone
> On Jun 14, 2017, at 8:44 AM, Bruce Schuchardt wrote:
>
> Having to deal with a github repo instead of branches off of the Apache repo
> is an additional burden on committers.
As a committee you have to work from the github repo to merge PRs from
non-committers. Having
I have to agree with Bruce here. This is an open community, and as such,
dictating whether or not we can use reviewboard for reviews works against the
openness. Furthermore, reviewboard is NOT restricted to committers - I
submitted many reviews using RB before becoming a committer. Once the revi
Having to deal with a github repo instead of branches off of the Apache
repo is an additional burden on committers.
I still vote -1 and don't see a lot of support for this idea.
On 6/13/17 3:03 PM, Jacob Barrett wrote:
On Mon, Jun 12, 2017 at 7:57 AM Bruce Schuchardt
wrote:
It places an un
On Mon, Jun 12, 2017 at 4:20 PM Udo Kohlmeyer
wrote:\
> * -1 for PR: I fear that given the huge amount of incoming PR's we
> might loose sight/oversight of any PR's that need to be approved
> from external submitters.
How does having to check review board and PR help this situation?
On Mon, Jun 12, 2017 at 9:37 AM Dave Barnes wrote:
> Proposal as written says "...for changes that would require peer review
> before committing...".
> If this means that minor changes (in my case, typo repairs are the common
> case) can be checked in without going through a PR process, I can go
On Mon, Jun 12, 2017 at 7:57 AM Bruce Schuchardt
wrote:
> It places an unnecessary burden on committers
Considering committers need to use PR to commit changes from non-committers
how does reducing the number of review systems increase the burden on
committers?
> and git history is the
> defi
t;>> definitive record of changes to the code so github pr history isn't
>>>>
>>> really
>>>
>>>> very useful. Also, Review Board is a much better tool for reviewing
>>>> code
>>>> than a github PR.
>>>>
>>
personal forks on
GitHub rather than directly on the ASF Geode repositories.
* Submit GitHub pull requests, following all current rules for pull
requests, for changes that would require peer review before committing
to
the production branches.
* Register their GitHub accounts with the ASF
rkflow of
> >> committers to match that of non-committers such that committers shall:
> >>
> >> * Perform all work in progress on branches in their personal forks on
> >> GitHub rather than directly on the ASF Geode repositories.
> >>
> >> * Su
gt;> like to take it to a vote. The proposal is to modify the workflow of
>> committers to match that of non-committers such that committers shall:
>>
>> * Perform all work in progress on branches in their personal forks on
>> GitHub rather than directly on the ASF Geod
than directly on the ASF Geode repositories.
* Submit GitHub pull requests, following all current rules for pull
requests, for changes that would require peer review before committing to
the production branches.
* Register their GitHub accounts with the ASF so that committers can be
identified i
rather than directly on the ASF Geode repositories.
* Submit GitHub pull requests, following all current rules for pull
requests, for changes that would require peer review before committing to
the production branches.
* Register their GitHub accounts with the ASF so that committers can be
identified
One feature I like about PRs on GitHub that I haven't figured out how to do
on Review Board is breaking up a large changeset (which I would like to
have reviewed together) into multiple commits. It can be a useful way to
tell a story about your changes, but keep the review for them in one place.
I
On Thu, Jun 8, 2017 at 2:24 PM Nabarun Nag wrote:
> Also, IMHO feature branches from which the PRs are created should be in our
> personal fork rather than the main geode git repo.
>
+1 - I had planned to bring this up in a separate discussion but yes, I
think all work should happen out of your
+1 on using PR.
We can use the @tags and the github notification page[Participating tag] to
check the PRs that need our attention.
Also, IMHO feature branches from which the PRs are created should be in our
personal fork rather than the main geode git repo. Because when we push a
branch called fea
On Thu, Jun 8, 2017 at 12:35 PM Mark Bretl wrote:
> I like the functionality we get with GitHub, including the Travis CI
> integration.
>
> Do we have a proposed workflow change for committers?
The proposed workflow change to committers is to use the same workflow as
contributors. The only diff
I like the functionality we get with GitHub, including the Travis CI
integration.
Do we have a proposed workflow change for committers?
--Mark
On Thu, Jun 8, 2017 at 11:20 AM, Jacob Barrett wrote:
> Some more fuel on this fire... PR's get checked against Travis CI
> automatically and the resul
Some more fuel on this fire... PR's get checked against Travis CI
automatically and the results show up in the PR. This is a good safety
check even for committers before merging their branch into the development
branch.
-Jake
On Thu, Jun 8, 2017 at 10:20 AM Dan Smith wrote:
> One thing that can
> help is if you add your github id to your apache profile - the PR will then
> show that it is coming from an apache member.
To illustrate what Dan is saying, notice the "Member" tag on my comment in
the screen cap below.
[i
+1 for just PRs
+1 for trying the functionality Jared has pointed out in Github
On Thu, Jun 8, 2017 at 10:19 AM, Jared Stewart wrote:
>
> On Jun 8, 2017, at 8:32 AM, Bruce Schuchardt
> wrote:
>
> I also don't see any way to push a PR to specific individuals for review.
> In Reviewboard there i
+1 to just using PRs.
I think the benefits to new people to the project make it worth it to
switch. New people will see PRs from committers when they are creating
their PRs, which will help provide good examples. It's one less system to
sign up on as a contributor so it's easier for new people to
On Thu, Jun 8, 2017 at 8:32 AM Bruce Schuchardt
wrote:
> One thing I find confusing in PRs is whether the person submitting the
> request is a committer or not. Non-committers need someone to merge the
> PR while committers are just looking for a review and will merge the
> changes to develop th
the idea of having to create PRs on a read-only system and then
> merge
> > my changes to ASF's repo. Being able to commit directly seems like a
> > committer perk that your idea would take away from us.
> >
> >
> > On 6/7/17 6:42 PM, Jacob Barrett wrote:
&g
On Thu, Jun 8, 2017 at 9:28 AM John Blum wrote:
> PRs are for contributors that do not have commit privileges. ReviewBoard
> is a tool for "reviewing" changes.
>
This isn't some law, it was our choice. What I am proposing is that we
re-evaluate this choice for consistency.
> However, what is
;s repo. Being able to commit directly seems like a
> committer perk that your idea would take away from us.
>
>
> On 6/7/17 6:42 PM, Jacob Barrett wrote:
>
>> All,
>>
>> I would like to discuss transitioning all code reviews to pull requests
>> over using revi
oning all code reviews to pull requests
over using review board. For non-committer community members we ask them to
make pull requests against the mirrored GitHub repo. Some committers use
pull requests for their own work reviews while others use review board. I
propose that we just use on and th
All,
I would like to discuss transitioning all code reviews to pull requests
over using review board. For non-committer community members we ask them to
make pull requests against the mirrored GitHub repo. Some committers use
pull requests for their own work reviews while others use review board
Looks like there are several pull requests that need some attention:
https://github.com/apache/geode/pulls
73 matches
Mail list logo
