Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

change import paths to kubernetes-sigs #2652

Merged
merged 1 commit into from Jun 5, 2019

Conversation

jberkhahn
Copy link
Contributor

@jberkhahn jberkhahn commented Jun 4, 2019

per #2650

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 4, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jberkhahn

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 4, 2019
@jberkhahn jberkhahn force-pushed the change_imports branch 5 times, most recently from 855acc3 to 086eb18 Compare June 4, 2019 23:19
@jberkhahn jberkhahn requested a review from mszostok June 5, 2019 16:30
Copy link
Contributor

@mszostok mszostok left a comment

Choose a reason for hiding this comment

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

Minor things that need to be addressed:

@mszostok
Copy link
Contributor

mszostok commented Jun 5, 2019

one thing is quite strange

When you click the Details button for pull-service-catalog-integration then the URL is:
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_service-catalog/2652/pull-service-catalog-xbuild/1136353016471883777/

and proper link should be:
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_service-catalog/2652/pull-service-catalog-xbuild/1136353016471883777/

Same situation for the pull-service-catalog-xbuild job. Only the pull-service-catalog-unit has the correct URL

I've check also the TEST PR and there is the same situation, so changes introduced in that PR didn't break that. Maybe sth is incorrect in test-infra repo config?

@mszostok
Copy link
Contributor

mszostok commented Jun 5, 2019

ok found it! after a 2h of debugging...

the problem is that we are using the old approach for pushing the logs. Service Catalog jobs are using the boostrap.py script. This script checkouts the repo and after all also pushes the logs into GCS. But link to job logs are set by the plank which has the job_url_template variable: https://github.com/kubernetes/test-infra/blob/master/prow/config.yaml#L2

If you will check that tmpl in details you will find such if statements:

{{if .Spec.Refs}}
    {{if ne .Spec.Refs.Org ""}}
        {{if ne .Spec.Refs.Org "kubernetes"}}/
            {{if and (eq .Spec.Refs.Org "kubernetes-sigs") (ne .Spec.Refs.Repo "poseidon")}}
				sigs.k8s.io
            {{else}}
				{{.Spec.Refs.Org}}
            {{end}}
        _{{.Spec.Refs.Repo}}
        {{else if ne .Spec.Refs.Repo "kubernetes"}}
			/{{.Spec.Refs.Repo}}
		{{end}}
    {{end}}
{{end}}

so unfortunately in templeate, we are entering in such if statement
{{if and (eq .Spec.Refs.Org "kubernetes-sigs") (ne .Spec.Refs.Repo "poseidon")}}
and the sigs.k8s.io suffix is added instead of {{.Spec.Refs.Org}}, where the logs are actually pushed by the boostrap.py script.

The problem is that the logic is decoupled, one tool is pushing the logs, other is setting the logs URL in ProwJob status and the logic is not the same.

The boostrap.py script is already deprecated. To solve that problem we should use the decorator pattern, same as we have for the pull-service-catalog-unit job. Thanks to that decorator will take care both for pushing the logs and setting proper URL in ProwJob status.

Here is more info about deprecated boostrap.py

Here is the PR to fix that: kubernetes/test-infra#12911


so your PR LGTM

@jberkhahn
Copy link
Contributor Author

yeah, i had just about figured that out myself. could you actually mark this as LGTM so it gets merged?

@mszostok
Copy link
Contributor

mszostok commented Jun 5, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2019
@jberkhahn
Copy link
Contributor Author

huh, the travis tests aren't reporting back for some reason.

/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2019
@jberkhahn
Copy link
Contributor Author

had to rejigger things to get it to run the tests again, so it knocked your LGTM off

@mszostok
Copy link
Contributor

mszostok commented Jun 5, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2019
@k8s-ci-robot k8s-ci-robot merged commit 492c017 into kubernetes-retired:master Jun 5, 2019
@MHBauer
Copy link
Contributor

MHBauer commented Jun 6, 2019

amazing

viviyww pushed a commit to viviyww/service-catalog that referenced this pull request Jun 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants