You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Bruce Schuchardt <br...@vmware.com> on 2021/02/19 19:30:34 UTC
code-owners seems to have some problems
I was pulled in to PR 5989<https://github.com/apache/geode/pull/5989> but can’t figure out how that happened with the current CODEOWNERS file. These all seem out of my area:
boms/geode-all-bom/src/test/resources/expected-pom.xml
buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
extensions/geode-modules-session/build.gradle
extensions/geode-modules/build.gradle
extensions/geode-modules/src/test/resources/expected-pom.xml
extensions/session-testing-war/build.gradle
...embly/geode-assembly-test/src/main/java/org/apache/geode/session/tests/TomcatInstall.java
geode-assembly/src/acceptanceTest/java/org/apache/geode/modules/DeployJarAcceptanceTest.java
...butedTest/java/org/apache/geode/management/internal/rest/DeployToMultiGroupDUnitTest.java
...tedTest/java/org/apache/geode/management/internal/rest/DeploymentManagementDUnitTest.java
.../java/org/apache/geode/management/internal/rest/DeploymentManagementRedployDUnitTest.java
...java/org/apache/geode/management/internal/rest/DeploymentSemanticVersionJarDUnitTest.java
geode-assembly/src/integrationTest/resources/assembly_content.txt
geode-assembly/src/integrationTest/resources/dependency_classpath.txt
.../java/org/apache/geode/connectors/jdbc/internal/cli/CreateDataSourceCommandDUnitTest.java
geode-core/build.gradle
...a/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java
geode-core/src/integrationTest/java/org/apache/geode/internal/ClassPathLoaderDeployTest.java
Re: code-owners seems to have some problems
Posted by Bruce Schuchardt <br...@vmware.com>.
Thanks Owen. Good to know about the keyhole/shield icons. The filter function doesn't work as well as I hoped. The shield icons that matched my username in CODEOWNERS are BOLD while the others are a bit greyed out;
Using that I was able to identify a few files that matched line 28 in CODEOWNERS.
#28: geode-core/**/org/apache/geode/internal/* @bschuchardt @Bill @kirklund
So that explains why I was added as a reviewer. Thanks
Re: code-owners seems to have some problems
Posted by Mark Hanson <ha...@vmware.com>.
Hi Robert,
Thanks! You answered the question the way I intended.
Thanks,
Mark
On 2/25/21, 12:57 PM, "Robert Houghton" <rh...@vmware.com> wrote:
Hi Mark,
Yes, each area of ownership only needs one review-by-owner.
Your following question, I do not understand, but I'll try: Any PR that needs review, will have text in the "reviewers needed and status-required" area, that states who is still needed to review. However, there is not a direct way to see if a co-owner has already reviewed a file you are looking at.
-Robert
On 2/25/21, 10:36 AM, "Mark Hanson" <ha...@vmware.com> wrote:
Hi Owen,
Two more questions.
Is it the case that only one of the code owners for that area has to review the PR?
Also, is there a way to tell that a code owner has reviewed an area, so you don't have to as one of the other code owners?
Thanks,
Mark
On 2/25/21, 10:31 AM, "Owen Nichols" <on...@vmware.com> wrote:
GitHub does not add reviewers from CODEOWNERS to PRs created as draft PRs (until you mark it not-draft by clicking Ready For Review). However if you subsequently change it back to a draft, GitHub will not remove any reviewers.
On 2/25/21, 10:28 AM, "Mark Hanson" <ha...@vmware.com> wrote:
Hi Owen,
Is there a way to ensure that draft mode PRs aren't requesting reviews from code owners?
Thanks,
Mark
On 2/19/21, 11:44 AM, "Owen Nichols" <on...@vmware.com> wrote:
GitHub provides some tools to answer this kind of question.
Step 1: Under the PR's "Files Changed" tab, click File Filter > Your CODEOWNER files
Step 2: Hover over the keyhole/shield icon (to the left of the red/green changecount graphic to the left of each filename) to see who else is also owner.
Step 3: If the ownership doesn't seem quite right, click on the keyhole/shield icon to see the exact line in CODEOWNERS that was applied.
Please note, if a file has NO codeowner, it will still show up in the "Your CODEOWNER files" (but no keyhole/shield indicator will be displayed). It would be fantastic to find owners for these remaining unowned files, but until then owned-by-no-one == owned-by-everyone.
Improvements to CODEOWNERS can always be submitted via PR.
Some files in your list below have no owner, but several are owned by someone else so they shouldn't be showing up in your list. Perhaps you copy&pasted this list from the Jump To dropdown? If so, it seems that a File Filter selection does not filter the Jump To dropdown, only the display below.
On 2/19/21, 11:30 AM, "Bruce Schuchardt" <br...@vmware.com> wrote:
I was pulled in to PR 5989 but can’t figure out how that happened with the current CODEOWNERS file. These all seem out of my area:
boms/geode-all-bom/src/test/resources/expected-pom.xml
buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
extensions/geode-modules-session/build.gradle
extensions/geode-modules/build.gradle
extensions/geode-modules/src/test/resources/expected-pom.xml
extensions/session-testing-war/build.gradle
...embly/geode-assembly-test/src/main/java/org/apache/geode/session/tests/TomcatInstall.java
geode-assembly/src/acceptanceTest/java/org/apache/geode/modules/DeployJarAcceptanceTest.java
...butedTest/java/org/apache/geode/management/internal/rest/DeployToMultiGroupDUnitTest.java
...tedTest/java/org/apache/geode/management/internal/rest/DeploymentManagementDUnitTest.java
.../java/org/apache/geode/management/internal/rest/DeploymentManagementRedployDUnitTest.java
...java/org/apache/geode/management/internal/rest/DeploymentSemanticVersionJarDUnitTest.java
geode-assembly/src/integrationTest/resources/assembly_content.txt
geode-assembly/src/integrationTest/resources/dependency_classpath.txt
.../java/org/apache/geode/connectors/jdbc/internal/cli/CreateDataSourceCommandDUnitTest.java
geode-core/build.gradle
...a/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java
geode-core/src/integrationTest/java/org/apache/geode/internal/ClassPathLoaderDeployTest.java
Re: code-owners seems to have some problems
Posted by Robert Houghton <rh...@vmware.com>.
Hi Mark,
Yes, each area of ownership only needs one review-by-owner.
Your following question, I do not understand, but I'll try: Any PR that needs review, will have text in the "reviewers needed and status-required" area, that states who is still needed to review. However, there is not a direct way to see if a co-owner has already reviewed a file you are looking at.
-Robert
On 2/25/21, 10:36 AM, "Mark Hanson" <ha...@vmware.com> wrote:
Hi Owen,
Two more questions.
Is it the case that only one of the code owners for that area has to review the PR?
Also, is there a way to tell that a code owner has reviewed an area, so you don't have to as one of the other code owners?
Thanks,
Mark
On 2/25/21, 10:31 AM, "Owen Nichols" <on...@vmware.com> wrote:
GitHub does not add reviewers from CODEOWNERS to PRs created as draft PRs (until you mark it not-draft by clicking Ready For Review). However if you subsequently change it back to a draft, GitHub will not remove any reviewers.
On 2/25/21, 10:28 AM, "Mark Hanson" <ha...@vmware.com> wrote:
Hi Owen,
Is there a way to ensure that draft mode PRs aren't requesting reviews from code owners?
Thanks,
Mark
On 2/19/21, 11:44 AM, "Owen Nichols" <on...@vmware.com> wrote:
GitHub provides some tools to answer this kind of question.
Step 1: Under the PR's "Files Changed" tab, click File Filter > Your CODEOWNER files
Step 2: Hover over the keyhole/shield icon (to the left of the red/green changecount graphic to the left of each filename) to see who else is also owner.
Step 3: If the ownership doesn't seem quite right, click on the keyhole/shield icon to see the exact line in CODEOWNERS that was applied.
Please note, if a file has NO codeowner, it will still show up in the "Your CODEOWNER files" (but no keyhole/shield indicator will be displayed). It would be fantastic to find owners for these remaining unowned files, but until then owned-by-no-one == owned-by-everyone.
Improvements to CODEOWNERS can always be submitted via PR.
Some files in your list below have no owner, but several are owned by someone else so they shouldn't be showing up in your list. Perhaps you copy&pasted this list from the Jump To dropdown? If so, it seems that a File Filter selection does not filter the Jump To dropdown, only the display below.
On 2/19/21, 11:30 AM, "Bruce Schuchardt" <br...@vmware.com> wrote:
I was pulled in to PR 5989 but can’t figure out how that happened with the current CODEOWNERS file. These all seem out of my area:
boms/geode-all-bom/src/test/resources/expected-pom.xml
buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
extensions/geode-modules-session/build.gradle
extensions/geode-modules/build.gradle
extensions/geode-modules/src/test/resources/expected-pom.xml
extensions/session-testing-war/build.gradle
...embly/geode-assembly-test/src/main/java/org/apache/geode/session/tests/TomcatInstall.java
geode-assembly/src/acceptanceTest/java/org/apache/geode/modules/DeployJarAcceptanceTest.java
...butedTest/java/org/apache/geode/management/internal/rest/DeployToMultiGroupDUnitTest.java
...tedTest/java/org/apache/geode/management/internal/rest/DeploymentManagementDUnitTest.java
.../java/org/apache/geode/management/internal/rest/DeploymentManagementRedployDUnitTest.java
...java/org/apache/geode/management/internal/rest/DeploymentSemanticVersionJarDUnitTest.java
geode-assembly/src/integrationTest/resources/assembly_content.txt
geode-assembly/src/integrationTest/resources/dependency_classpath.txt
.../java/org/apache/geode/connectors/jdbc/internal/cli/CreateDataSourceCommandDUnitTest.java
geode-core/build.gradle
...a/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java
geode-core/src/integrationTest/java/org/apache/geode/internal/ClassPathLoaderDeployTest.java
Re: code-owners seems to have some problems
Posted by Mark Hanson <ha...@vmware.com>.
Hi Owen,
Two more questions.
Is it the case that only one of the code owners for that area has to review the PR?
Also, is there a way to tell that a code owner has reviewed an area, so you don't have to as one of the other code owners?
Thanks,
Mark
On 2/25/21, 10:31 AM, "Owen Nichols" <on...@vmware.com> wrote:
GitHub does not add reviewers from CODEOWNERS to PRs created as draft PRs (until you mark it not-draft by clicking Ready For Review). However if you subsequently change it back to a draft, GitHub will not remove any reviewers.
On 2/25/21, 10:28 AM, "Mark Hanson" <ha...@vmware.com> wrote:
Hi Owen,
Is there a way to ensure that draft mode PRs aren't requesting reviews from code owners?
Thanks,
Mark
On 2/19/21, 11:44 AM, "Owen Nichols" <on...@vmware.com> wrote:
GitHub provides some tools to answer this kind of question.
Step 1: Under the PR's "Files Changed" tab, click File Filter > Your CODEOWNER files
Step 2: Hover over the keyhole/shield icon (to the left of the red/green changecount graphic to the left of each filename) to see who else is also owner.
Step 3: If the ownership doesn't seem quite right, click on the keyhole/shield icon to see the exact line in CODEOWNERS that was applied.
Please note, if a file has NO codeowner, it will still show up in the "Your CODEOWNER files" (but no keyhole/shield indicator will be displayed). It would be fantastic to find owners for these remaining unowned files, but until then owned-by-no-one == owned-by-everyone.
Improvements to CODEOWNERS can always be submitted via PR.
Some files in your list below have no owner, but several are owned by someone else so they shouldn't be showing up in your list. Perhaps you copy&pasted this list from the Jump To dropdown? If so, it seems that a File Filter selection does not filter the Jump To dropdown, only the display below.
On 2/19/21, 11:30 AM, "Bruce Schuchardt" <br...@vmware.com> wrote:
I was pulled in to PR 5989 but can’t figure out how that happened with the current CODEOWNERS file. These all seem out of my area:
boms/geode-all-bom/src/test/resources/expected-pom.xml
buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
extensions/geode-modules-session/build.gradle
extensions/geode-modules/build.gradle
extensions/geode-modules/src/test/resources/expected-pom.xml
extensions/session-testing-war/build.gradle
...embly/geode-assembly-test/src/main/java/org/apache/geode/session/tests/TomcatInstall.java
geode-assembly/src/acceptanceTest/java/org/apache/geode/modules/DeployJarAcceptanceTest.java
...butedTest/java/org/apache/geode/management/internal/rest/DeployToMultiGroupDUnitTest.java
...tedTest/java/org/apache/geode/management/internal/rest/DeploymentManagementDUnitTest.java
.../java/org/apache/geode/management/internal/rest/DeploymentManagementRedployDUnitTest.java
...java/org/apache/geode/management/internal/rest/DeploymentSemanticVersionJarDUnitTest.java
geode-assembly/src/integrationTest/resources/assembly_content.txt
geode-assembly/src/integrationTest/resources/dependency_classpath.txt
.../java/org/apache/geode/connectors/jdbc/internal/cli/CreateDataSourceCommandDUnitTest.java
geode-core/build.gradle
...a/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java
geode-core/src/integrationTest/java/org/apache/geode/internal/ClassPathLoaderDeployTest.java
Re: code-owners seems to have some problems
Posted by Owen Nichols <on...@vmware.com>.
GitHub does not add reviewers from CODEOWNERS to PRs created as draft PRs (until you mark it not-draft by clicking Ready For Review). However if you subsequently change it back to a draft, GitHub will not remove any reviewers.
On 2/25/21, 10:28 AM, "Mark Hanson" <ha...@vmware.com> wrote:
Hi Owen,
Is there a way to ensure that draft mode PRs aren't requesting reviews from code owners?
Thanks,
Mark
On 2/19/21, 11:44 AM, "Owen Nichols" <on...@vmware.com> wrote:
GitHub provides some tools to answer this kind of question.
Step 1: Under the PR's "Files Changed" tab, click File Filter > Your CODEOWNER files
Step 2: Hover over the keyhole/shield icon (to the left of the red/green changecount graphic to the left of each filename) to see who else is also owner.
Step 3: If the ownership doesn't seem quite right, click on the keyhole/shield icon to see the exact line in CODEOWNERS that was applied.
Please note, if a file has NO codeowner, it will still show up in the "Your CODEOWNER files" (but no keyhole/shield indicator will be displayed). It would be fantastic to find owners for these remaining unowned files, but until then owned-by-no-one == owned-by-everyone.
Improvements to CODEOWNERS can always be submitted via PR.
Some files in your list below have no owner, but several are owned by someone else so they shouldn't be showing up in your list. Perhaps you copy&pasted this list from the Jump To dropdown? If so, it seems that a File Filter selection does not filter the Jump To dropdown, only the display below.
On 2/19/21, 11:30 AM, "Bruce Schuchardt" <br...@vmware.com> wrote:
I was pulled in to PR 5989 but can’t figure out how that happened with the current CODEOWNERS file. These all seem out of my area:
boms/geode-all-bom/src/test/resources/expected-pom.xml
buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
extensions/geode-modules-session/build.gradle
extensions/geode-modules/build.gradle
extensions/geode-modules/src/test/resources/expected-pom.xml
extensions/session-testing-war/build.gradle
...embly/geode-assembly-test/src/main/java/org/apache/geode/session/tests/TomcatInstall.java
geode-assembly/src/acceptanceTest/java/org/apache/geode/modules/DeployJarAcceptanceTest.java
...butedTest/java/org/apache/geode/management/internal/rest/DeployToMultiGroupDUnitTest.java
...tedTest/java/org/apache/geode/management/internal/rest/DeploymentManagementDUnitTest.java
.../java/org/apache/geode/management/internal/rest/DeploymentManagementRedployDUnitTest.java
...java/org/apache/geode/management/internal/rest/DeploymentSemanticVersionJarDUnitTest.java
geode-assembly/src/integrationTest/resources/assembly_content.txt
geode-assembly/src/integrationTest/resources/dependency_classpath.txt
.../java/org/apache/geode/connectors/jdbc/internal/cli/CreateDataSourceCommandDUnitTest.java
geode-core/build.gradle
...a/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java
geode-core/src/integrationTest/java/org/apache/geode/internal/ClassPathLoaderDeployTest.java
Re: code-owners seems to have some problems
Posted by Mark Hanson <ha...@vmware.com>.
Hi Owen,
Is there a way to ensure that draft mode PRs aren't requesting reviews from code owners?
Thanks,
Mark
On 2/19/21, 11:44 AM, "Owen Nichols" <on...@vmware.com> wrote:
GitHub provides some tools to answer this kind of question.
Step 1: Under the PR's "Files Changed" tab, click File Filter > Your CODEOWNER files
Step 2: Hover over the keyhole/shield icon (to the left of the red/green changecount graphic to the left of each filename) to see who else is also owner.
Step 3: If the ownership doesn't seem quite right, click on the keyhole/shield icon to see the exact line in CODEOWNERS that was applied.
Please note, if a file has NO codeowner, it will still show up in the "Your CODEOWNER files" (but no keyhole/shield indicator will be displayed). It would be fantastic to find owners for these remaining unowned files, but until then owned-by-no-one == owned-by-everyone.
Improvements to CODEOWNERS can always be submitted via PR.
Some files in your list below have no owner, but several are owned by someone else so they shouldn't be showing up in your list. Perhaps you copy&pasted this list from the Jump To dropdown? If so, it seems that a File Filter selection does not filter the Jump To dropdown, only the display below.
On 2/19/21, 11:30 AM, "Bruce Schuchardt" <br...@vmware.com> wrote:
I was pulled in to PR 5989 but can’t figure out how that happened with the current CODEOWNERS file. These all seem out of my area:
boms/geode-all-bom/src/test/resources/expected-pom.xml
buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
extensions/geode-modules-session/build.gradle
extensions/geode-modules/build.gradle
extensions/geode-modules/src/test/resources/expected-pom.xml
extensions/session-testing-war/build.gradle
...embly/geode-assembly-test/src/main/java/org/apache/geode/session/tests/TomcatInstall.java
geode-assembly/src/acceptanceTest/java/org/apache/geode/modules/DeployJarAcceptanceTest.java
...butedTest/java/org/apache/geode/management/internal/rest/DeployToMultiGroupDUnitTest.java
...tedTest/java/org/apache/geode/management/internal/rest/DeploymentManagementDUnitTest.java
.../java/org/apache/geode/management/internal/rest/DeploymentManagementRedployDUnitTest.java
...java/org/apache/geode/management/internal/rest/DeploymentSemanticVersionJarDUnitTest.java
geode-assembly/src/integrationTest/resources/assembly_content.txt
geode-assembly/src/integrationTest/resources/dependency_classpath.txt
.../java/org/apache/geode/connectors/jdbc/internal/cli/CreateDataSourceCommandDUnitTest.java
geode-core/build.gradle
...a/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java
geode-core/src/integrationTest/java/org/apache/geode/internal/ClassPathLoaderDeployTest.java
Re: code-owners seems to have some problems
Posted by Owen Nichols <on...@vmware.com>.
GitHub provides some tools to answer this kind of question.
Step 1: Under the PR's "Files Changed" tab, click File Filter > Your CODEOWNER files
Step 2: Hover over the keyhole/shield icon (to the left of the red/green changecount graphic to the left of each filename) to see who else is also owner.
Step 3: If the ownership doesn't seem quite right, click on the keyhole/shield icon to see the exact line in CODEOWNERS that was applied.
Please note, if a file has NO codeowner, it will still show up in the "Your CODEOWNER files" (but no keyhole/shield indicator will be displayed). It would be fantastic to find owners for these remaining unowned files, but until then owned-by-no-one == owned-by-everyone.
Improvements to CODEOWNERS can always be submitted via PR.
Some files in your list below have no owner, but several are owned by someone else so they shouldn't be showing up in your list. Perhaps you copy&pasted this list from the Jump To dropdown? If so, it seems that a File Filter selection does not filter the Jump To dropdown, only the display below.
On 2/19/21, 11:30 AM, "Bruce Schuchardt" <br...@vmware.com> wrote:
I was pulled in to PR 5989 but can’t figure out how that happened with the current CODEOWNERS file. These all seem out of my area:
boms/geode-all-bom/src/test/resources/expected-pom.xml
buildSrc/src/main/groovy/org/apache/geode/gradle/plugins/DependencyConstraints.groovy
extensions/geode-modules-session/build.gradle
extensions/geode-modules/build.gradle
extensions/geode-modules/src/test/resources/expected-pom.xml
extensions/session-testing-war/build.gradle
...embly/geode-assembly-test/src/main/java/org/apache/geode/session/tests/TomcatInstall.java
geode-assembly/src/acceptanceTest/java/org/apache/geode/modules/DeployJarAcceptanceTest.java
...butedTest/java/org/apache/geode/management/internal/rest/DeployToMultiGroupDUnitTest.java
...tedTest/java/org/apache/geode/management/internal/rest/DeploymentManagementDUnitTest.java
.../java/org/apache/geode/management/internal/rest/DeploymentManagementRedployDUnitTest.java
...java/org/apache/geode/management/internal/rest/DeploymentSemanticVersionJarDUnitTest.java
geode-assembly/src/integrationTest/resources/assembly_content.txt
geode-assembly/src/integrationTest/resources/dependency_classpath.txt
.../java/org/apache/geode/connectors/jdbc/internal/cli/CreateDataSourceCommandDUnitTest.java
geode-core/build.gradle
...a/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java
geode-core/src/integrationTest/java/org/apache/geode/internal/ClassPathLoaderDeployTest.java