-
Couldn't load subscription status.
- Fork 289
multi-pr-prow-plugin: Determine path_alias when testing heterogeneous PRs #4769
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
base: main
Are you sure you want to change the base?
multi-pr-prow-plugin: Determine path_alias when testing heterogeneous PRs #4769
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli 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 |
98ad8e0 to
d386355
Compare
cmd/multi-pr-prow-plugin/server.go
Outdated
| } | ||
|
|
||
| pathAliasFor := func(org, repo string) string { | ||
| if org == ciopConfig.Metadata.Org && repo == ciopConfig.Metadata.Repo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smg247 not sure whether we should take the branch name into account here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it matters in practice. It is very unlikely that one branch will be configured to use a different canonical_go_repository than the others. We won't likely ever run into a problem with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am worried that this won't get the proper path alias for the non-originating PR though when the canonical_go_repository is configured there. Have you tested this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, it won't. I did it on purpose because I wasn't sure of how to handle the problem without making an intrusive change (which I still don't want to do unless we realize that this thing is fundamentally broken).
Suppose the following command:
/testwith openshift/kubernetes/master/okd-scos/e2e openshift/installer#999
The ci-operator config of the originating PR is fully determined:
openshift-kubernetes-master__okd-scos.yaml
so it is the canonical_go_repository.
But what about openshift/installer#999? We should grab the baseRef (assuming main here for simplicity) from the PR and then load:
openshift-installer-main.yaml
but we are still missing the information about the variant, which we do have on the originating PR.
Or we could just always use this pattern and forget about variants 🤷🏻 :
<ORG>-<REPO>-<BASE_REF>.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variants can be ignored. I think there are some subtle bugs around them, but they aren't widely used. Neither is the canonical_go_repository, but I think it is easier to solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you ok with this proposal then or you want me to load the "main" ci-operator configs of any additional PRs and use the canonical_go_repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to take the other canonical_go_repository configs into account, yes. I was hoping that we could do that in the config merging logic.
The idea there would be that the merged config contains all relevant canonical_go_repository values in the list, and then ci-operator knows how to find the right one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now resolving the canonical_go_repository by looking at the ci-operator configuration, for every PR, whether it is an originating one or not. Does it make sense?
d386355 to
21e7e00
Compare
|
@danilo-gemoli: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Revert this #4755 because we are still having troubles.
When a test is run against heterogeneous PRs (see this example) the
path_aliasof any PR is set in accordance with theci-operatorconfiguration of the originating PR.This approach is problematic for several reasons:
gitfails when cloning two repositories within the same directoryThis PR tries to address the problem by using the
ci-operatorconfig'scanonical_go_repositoryfield aspath_aliasfor any PR whose organization and repository matchingzz_generated_metadata.organdzz_generated_metadata.repo(for thatci-operatorconfig), leaving it empty""otherwise.Assuming the following configuration:
The tool retrieves this
ci-operatorconfig for the originating PR:ci-operator/config/openshift/cluster-api/openshift-cluster-api-master.yaml:then it generates this git ref for the originating PR:
and this for the additional one:
As a result,
clonerefsclones the repositories within the following directories:/go/src/sigs.k8s.io/cluster-apifor the originating PRhttps://github.com/openshift/cluster-api/pull/243/go/src/github.com/openshift/cluster-capi-operatorforhttps://github.com/openshift/cluster-capi-operator/pull/385/cc @deepsm007 @smg247