Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix typo in Github Bearer Token and remove redundant check for ignoring non-yaml files in config.go #1409

Merged
merged 1 commit into from Aug 10, 2020
Merged

Fix typo in Github Bearer Token and remove redundant check for ignoring non-yaml files in config.go #1409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 1 addition & 7 deletions perfdash/config.go
View file
Open in desktop
Expand Up @@ -23,9 +23,8 @@ import (
"strconv"
"strings"

"k8s.io/klog"

"github.com/ghodss/yaml"
"k8s.io/klog"
)

// To add new e2e test support, you need to:
Expand Down Expand Up @@ -407,11 +406,6 @@ func getProwConfig(configPaths []string) (Jobs, error) {

for _, configPath := range configPaths {
klog.Infof("Fetching config %s", configPath)
// Perfdash supports only yamls.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you're removing this one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't fit into this PR (and this is actually a useful check).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe adding a bearer token breaks the HasSuffix() check here since the downloadURL will look like https://raw.githubusercontent.com/kubernetes/test-infra/master/config/jobs/kubernetes/sig-scalability/sig-scalability-release-blocking-jobs.yaml?token=AXXXXXXXXXXXXXXXXXXXXZ

Isn't this a redundant check since we guarantee yaml files in GetConfigsFromGithub()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't fit into this PR

I can rebase and just push the typo fix if it doesn't fit :D Going to wait for your reply to my prev comment before I do :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - the explanation makes sense to me and looking into the code it seems to be indeed reduntant.

if !strings.HasSuffix(configPath, ".yaml") {
klog.Warningf("%s is not an yaml file!", configPath)
continue
}
var content []byte
var err error
switch {
Expand Down
4 changes: 2 additions & 2 deletions perfdash/github-configs-fetcher.go
View file
Open in desktop
Expand Up @@ -19,12 +19,12 @@ package main
import (
"fmt"
"io/ioutil"
"k8s.io/klog"
"net/http"
"os"
"strings"

"gopkg.in/yaml.v2"
"k8s.io/klog"
)

type githubDirContent struct {
Expand Down Expand Up @@ -65,7 +65,7 @@ func getGithubDirContents(url string) ([]githubDirContent, error) {
return nil, err
}
if token := os.Getenv("GITHUB_TOKEN"); len(token) != 0 {
req.Header.Add("Authorization", fmt.Sprintf("Bearer: %s", token))
req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", token))
}
client := &http.Client{}
resp, err := client.Do(req)
Expand Down