covbot could handle git mv better

mattmoor commented 6 years ago

When I git mv a file, it shows as not having a baseline, which isn't the case for git mv (e.g.)

cc @jessiezcc

steuhs commented 6 years ago

@cjwagner do you know which golang git library kubernetes/test-infra uses?

cjwagner commented 6 years ago

We don't use a specific golang git library throughout kubernetes/test-infra. I think most places just shell out to git. Prow does have a git client library written in golang that you can reference, but I don't think it will help with this problem.

You can handle this case by using more of the data that is provided when you list the files changed by a pull request. The example PR @mattmoor provided returns the following when you list the file changes (see the status and previous_filename fields):

[
  {
    "sha": "b35a1e8aaaeba9ae127bf91e284219ac778bdee2",
    "filename": "pkg/controller/route/config/doc.go",
    "status": "added",
    "additions": 19,
    "deletions": 0,
    "changes": 19,
    "blob_url": "https://github.com/knative/serving/blob/e9656d7116fa8956a16a1c7b0254f81aa3510f97/pkg/controller/route/config/doc.go",
    "raw_url": "https://github.com/knative/serving/raw/e9656d7116fa8956a16a1c7b0254f81aa3510f97/pkg/controller/route/config/doc.go",
    "contents_url": "https://api.github.com/repos/knative/serving/contents/pkg/controller/route/config/doc.go?ref=e9656d7116fa8956a16a1c7b0254f81aa3510f97",
    "patch": "@@ -0,0 +1,19 @@\n+/*\n+Copyright 2018 The Knative Authors\n+\n+Licensed under the Apache License, Version 2.0 (the \"License\");\n+you may not use this file except in compliance with the License.\n+You may obtain a copy of the License at\n+\n+    http://www.apache.org/licenses/LICENSE-2.0\n+\n+Unless required by applicable law or agreed to in writing, software\n+distributed under the License is distributed on an \"AS IS\" BASIS,\n+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\n+See the License for the specific language governing permissions and\n+limitations under the License.\n+*/\n+\n+// Package config holds the typed objects that define the schemas for\n+// assorted ConfigMap objects on which the Route controller depends.\n+package config"
  },
  {
    "sha": "2246e5cf4809271b6dcccedb1aa721899fc5aa9d",
    "filename": "pkg/controller/route/config/domain.go",
    "status": "renamed",
    "additions": 7,
    "deletions": 7,
    "changes": 14,
    "blob_url": "https://github.com/knative/serving/blob/e9656d7116fa8956a16a1c7b0254f81aa3510f97/pkg/controller/route/config/domain.go",
    "raw_url": "https://github.com/knative/serving/raw/e9656d7116fa8956a16a1c7b0254f81aa3510f97/pkg/controller/route/config/domain.go",
    "contents_url": "https://api.github.com/repos/knative/serving/contents/pkg/controller/route/config/domain.go?ref=e9656d7116fa8956a16a1c7b0254f81aa3510f97",
    "patch": "@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and\n limitations under the License.\n */\n \n-package route\n+package config\n \n import (\n \t\"fmt\"\n@@ -45,19 +45,19 @@ func (s *LabelSelector) Matches(labels map[string]string) bool {\n \treturn true\n }\n \n-// DomainConfig maps domains to routes by matching the domain's\n+// Domain maps domains to routes by matching the domain's\n // label selectors to the route's labels.\n-type DomainConfig struct {\n+type Domain struct {\n \t// Domains map from domain to label selector.  If a route has\n \t// labels matching a particular selector, it will use the\n \t// corresponding domain.  If multiple selectors match, we choose\n \t// the most specific selector.\n \tDomains map[string]*LabelSelector\n }\n \n-// NewDomainConfigFromConfigMap creates a DomainConfig from the supplied ConfigMap\n-func NewDomainConfigFromConfigMap(configMap *corev1.ConfigMap) (*DomainConfig, error) {\n-\tc := DomainConfig{Domains: map[string]*LabelSelector{}}\n+// NewDomainFromConfigMap creates a Domain from the supplied ConfigMap\n+func NewDomainFromConfigMap(configMap *corev1.ConfigMap) (*Domain, error) {\n+\tc := Domain{Domains: map[string]*LabelSelector{}}\n \thasDefault := false\n \tfor k, v := range configMap.Data {\n \t\tlabelSelector := LabelSelector{}\n@@ -79,7 +79,7 @@ func NewDomainConfigFromConfigMap(configMap *corev1.ConfigMap) (*DomainConfig, e\n // LookupDomainForLabels returns a domain given a set of labels.\n // Since we reject configuration without a default domain, this should\n // always return a value.\n-func (c *DomainConfig) LookupDomainForLabels(labels map[string]string) string {\n+func (c *Domain) LookupDomainForLabels(labels map[string]string) string {\n \tdomain := \"\"\n \tspecificity := -1\n ",
    "previous_filename": "pkg/controller/route/domainconfig.go"
  },
  {
    "sha": "abefae9a34899fc88c8eefb37edf832962700355",
    "filename": "pkg/controller/route/config/domain_test.go",
    "status": "renamed",
    "additions": 23,
    "deletions": 8,
    "changes": 31,
    "blob_url": "https://github.com/knative/serving/blob/e9656d7116fa8956a16a1c7b0254f81aa3510f97/pkg/controller/route/config/domain_test.go",
    "raw_url": "https://github.com/knative/serving/raw/e9656d7116fa8956a16a1c7b0254f81aa3510f97/pkg/controller/route/config/domain_test.go",
    "contents_url": "https://api.github.com/repos/knative/serving/contents/pkg/controller/route/config/domain_test.go?ref=e9656d7116fa8956a16a1c7b0254f81aa3510f97",
    "patch": "@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and\n limitations under the License.\n */\n \n-package route\n+package config\n \n import (\n \t\"fmt\"\n@@ -60,7 +60,7 @@ func TestSelectorMatches(t *testing.T) {\n }\n \n func TestNewConfigNoEntry(t *testing.T) {\n-\t_, err := NewDomainConfigFromConfigMap(&corev1.ConfigMap{\n+\t_, err := NewDomainFromConfigMap(&corev1.ConfigMap{\n \t\tObjectMeta: metav1.ObjectMeta{\n \t\t\tNamespace: pkg.GetServingSystemNamespace(),\n \t\t\tName:      controller.GetDomainConfigMapName(),\n@@ -71,8 +71,23 @@ func TestNewConfigNoEntry(t *testing.T) {\n \t}\n }\n \n+func TestNewConfigBadYaml(t *testing.T) {\n+\tc, err := NewDomainFromConfigMap(&corev1.ConfigMap{\n+\t\tObjectMeta: metav1.ObjectMeta{\n+\t\t\tNamespace: pkg.GetServingSystemNamespace(),\n+\t\t\tName:      controller.GetDomainConfigMapName(),\n+\t\t},\n+\t\tData: map[string]string{\n+\t\t\t\"default.com\": \"bad: yaml: all: day\",\n+\t\t},\n+\t})\n+\tif err == nil {\n+\t\tt.Errorf(\"NewDomainFromConfigMap() = %v, wanted error\", c)\n+\t}\n+}\n+\n func TestNewConfig(t *testing.T) {\n-\texpectedConfig := DomainConfig{\n+\texpectedConfig := Domain{\n \t\tDomains: map[string]*LabelSelector{\n \t\t\t\"test-domain.foo.com\": {\n \t\t\t\tSelector: map[string]string{\n@@ -88,7 +103,7 @@ func TestNewConfig(t *testing.T) {\n \t\t\t\"default.com\": {},\n \t\t},\n \t}\n-\tc, err := NewDomainConfigFromConfigMap(&corev1.ConfigMap{\n+\tc, err := NewDomainFromConfigMap(&corev1.ConfigMap{\n \t\tObjectMeta: metav1.ObjectMeta{\n \t\t\tNamespace: pkg.GetServingSystemNamespace(),\n \t\t\tName:      controller.GetDomainConfigMapName(),\n@@ -108,7 +123,7 @@ func TestNewConfig(t *testing.T) {\n }\n \n func TestLookupDomainForLabels(t *testing.T) {\n-\tconfig := DomainConfig{\n+\tconfig := Domain{\n \t\tDomains: map[string]*LabelSelector{\n \t\t\t\"test-domain.foo.com\": {\n \t\t\t\tSelector: map[string]string{\n@@ -162,7 +177,7 @@ func TestLookupDomainForLabels(t *testing.T) {\n \t}\n }\n \n-func TestOurDomainConfig(t *testing.T) {\n+func TestOurDomain(t *testing.T) {\n \tb, err := ioutil.ReadFile(fmt.Sprintf(\"testdata/%s.yaml\", controller.GetDomainConfigMapName()))\n \tif err != nil {\n \t\tt.Errorf(\"ReadFile() = %v\", err)\n@@ -171,7 +186,7 @@ func TestOurDomainConfig(t *testing.T) {\n \tif err := yaml.Unmarshal(b, &cm); err != nil {\n \t\tt.Errorf(\"yaml.Unmarshal() = %v\", err)\n \t}\n-\tif _, err := NewDomainConfigFromConfigMap(&cm); err != nil {\n-\t\tt.Errorf(\"NewDomainConfigFromConfigMap() = %v\", err)\n+\tif _, err := NewDomainFromConfigMap(&cm); err != nil {\n+\t\tt.Errorf(\"NewDomainFromConfigMap() = %v\", err)\n \t}\n }",
    "previous_filename": "pkg/controller/route/domainconfig_test.go"
  },
  {
    "sha": "fd6402b7c48d100138d21b53a82d58d564c89fe1",
    "filename": "pkg/controller/route/config/testdata/config-domain.yaml",
    "status": "added",
    "additions": 1,
    "deletions": 0,
    "changes": 1,
    "blob_url": "https://github.com/knative/serving/blob/e9656d7116fa8956a16a1c7b0254f81aa3510f97/pkg/controller/route/config/testdata/config-domain.yaml",
    "raw_url": "https://github.com/knative/serving/raw/e9656d7116fa8956a16a1c7b0254f81aa3510f97/pkg/controller/route/config/testdata/config-domain.yaml",
    "contents_url": "https://api.github.com/repos/knative/serving/contents/pkg/controller/route/config/testdata/config-domain.yaml?ref=e9656d7116fa8956a16a1c7b0254f81aa3510f97",
    "patch": "@@ -0,0 +1 @@\n+../../../../../config/config-domain.yaml\n\\ No newline at end of file"
  },
  {
    "sha": "5a0f782149e06c424983a809a58c33649db5c229",
    "filename": "pkg/controller/route/route.go",
    "status": "modified",
    "additions": 4,
    "deletions": 3,
    "changes": 7,
    "blob_url": "https://github.com/knative/serving/blob/e9656d7116fa8956a16a1c7b0254f81aa3510f97/pkg/controller/route/route.go",
    "raw_url": "https://github.com/knative/serving/raw/e9656d7116fa8956a16a1c7b0254f81aa3510f97/pkg/controller/route/route.go",
    "contents_url": "https://api.github.com/repos/knative/serving/contents/pkg/controller/route/route.go?ref=e9656d7116fa8956a16a1c7b0254f81aa3510f97",
    "patch": "@@ -32,6 +32,7 @@ import (\n \tservinginformers \"github.com/knative/serving/pkg/client/informers/externalversions/serving/v1alpha1\"\n \tlisters \"github.com/knative/serving/pkg/client/listers/serving/v1alpha1\"\n \t\"github.com/knative/serving/pkg/controller\"\n+\t\"github.com/knative/serving/pkg/controller/route/config\"\n \t\"github.com/knative/serving/pkg/controller/route/resources\"\n \t\"github.com/knative/serving/pkg/controller/route/traffic\"\n \t\"github.com/knative/serving/pkg/logging\"\n@@ -52,7 +53,7 @@ type Controller struct {\n \n \t// Domain configuration could change over time and access to domainConfig\n \t// must go through domainConfigMutex\n-\tdomainConfig      *DomainConfig\n+\tdomainConfig      *config.Domain\n \tdomainConfigMutex sync.Mutex\n }\n \n@@ -232,7 +233,7 @@ func loggerWithRouteInfo(logger *zap.SugaredLogger, ns string, name string) *zap\n \treturn logger.With(zap.String(logkey.Namespace, ns), zap.String(logkey.Route, name))\n }\n \n-func (c *Controller) getDomainConfig() *DomainConfig {\n+func (c *Controller) getDomainConfig() *config.Domain {\n \tc.domainConfigMutex.Lock()\n \tdefer c.domainConfigMutex.Unlock()\n \treturn c.domainConfig\n@@ -244,7 +245,7 @@ func (c *Controller) routeDomain(route *v1alpha1.Route) string {\n }\n \n func (c *Controller) receiveDomainConfig(configMap *corev1.ConfigMap) {\n-\tnewDomainConfig, err := NewDomainConfigFromConfigMap(configMap)\n+\tnewDomainConfig, err := config.NewDomainFromConfigMap(configMap)\n \tif err != nil {\n \t\tc.Logger.Error(\"Failed to parse the new config map. Previous config map will be used.\",\n \t\t\tzap.Error(err))"
  },
  {
    "sha": "017ad94187217b6abddf167f8503f77df437a0f2",
    "filename": "pkg/controller/route/testdata/config-domain.yaml",
    "status": "removed",
    "additions": 0,
    "deletions": 1,
    "changes": 1,
    "blob_url": "https://github.com/knative/serving/blob/da830e76ecbd134852fd9a7312cbfdd6aaf9c034/pkg/controller/route/testdata/config-domain.yaml",
    "raw_url": "https://github.com/knative/serving/raw/da830e76ecbd134852fd9a7312cbfdd6aaf9c034/pkg/controller/route/testdata/config-domain.yaml",
    "contents_url": "https://api.github.com/repos/knative/serving/contents/pkg/controller/route/testdata/config-domain.yaml?ref=da830e76ecbd134852fd9a7312cbfdd6aaf9c034",
    "patch": "@@ -1 +0,0 @@\n-../../../../config/config-domain.yaml\n\\ No newline at end of file"
  }
]

mattmoor commented 5 years ago

Yes, the list of CommitFile objects in the PR object contain metadata showing moves.

steuhs commented 5 years ago

Closing this issue for the following reasons

This change is only suggested by one person over 5 months
The change requires access to either a. source code & git b. github client However, the new design(https://github.com/kubernetes/test-infra/pull/10010) will only take two coverage profiles as inputs. In that way it would not be feasible to make this required change.

knative / serving

covbot could handle git mv better #1466