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.

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

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

Conversation

mirahmed753
Copy link
Contributor

I was running into an auth issue despite setting my Github token properly. Upon investigation, I found that the request header for auth should look like: Authorization: Bearer <token>. In the current code, we have a colon after Bearer leading to my auth issues. Once removed, I was authenticated properly.

Unfortunately, this breaks retrieving the prow config because a bearer token adds some extra auth chars after the downloadURL. (e.g: https://raw.githubusercontent.com/kubernetes/test-infra/master/config/jobs/kubernetes/sig-scalability/sig-scalability-release-blocking-jobs.yaml?token=AXXXXXXXXXXXXXXXXXXXXZ). In the code:

In this PR:

  • I fix the bearer token typo
  • I remove the redundant check in getProwConfig() since we already guarantee we're working with only yaml files in the first check in GetConfigsFromGithub()

* Remove redundant check for ignoring non-yaml files in config.go
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 6, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @mirahmed753. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 6, 2020
@mirahmed753 mirahmed753 marked this pull request as ready for review August 6, 2020 22:15
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2020
@mirahmed753
Copy link
Contributor Author

/assign @wojtek-t

@@ -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.

@wojtek-t
Copy link
Member

/ok-to-test
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 10, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mirahmed753, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2020
@k8s-ci-robot k8s-ci-robot merged commit acff8b3 into kubernetes:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants