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

Allow configuring the Prow URL for command help #14045

Open

Conversation

@stevekuznetsov
Copy link
Contributor

commented Aug 23, 2019

Not all Prow deployments will use the configurations that prow.k8s.io
does, and it is best to link to the direct help page for the repo in
question for the deployment that is used to run it.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

/cc @spiffxp @cblecker @nikhita

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevekuznetsov

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

@@ -26,11 +26,13 @@ import (
// AboutThisBotWithoutCommands contains the message that explains how to interact with the bot.
const AboutThisBotWithoutCommands = "Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository."

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Aug 23, 2019

Author Contributor

FYI I noticed all of the helpers in this file use this one, without the command help. Why? Do we want to standardize on using the full footer?

@cblecker

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

It doesn't look like prow.k8s.io's config has the value of this set. Also, we should have a default value fallback, so that other consumers of prow can default to the current functionality, but with the ability to set their own URL.

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

Not super thrilled about having go.k8s.io as the fallback since it's wrong in most cases, but ok.

@cblecker

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

The other option would be to just link directly to ${deck_url}/command-help if deck is installed?

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

I don't think we have any way to know where deck is, without a configuration option like this. See also every config field in the main config struct that references the deck URL

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/configure-prow-url-response branch from b1fedd8 to 0f7741a Aug 30, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

@spiffxp @nikhita any thoughts?

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/configure-prow-url-response branch 2 times, most recently from a961c57 to 5a86c70 Aug 30, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

OK, this got super messy by adding it to the controllers for reporting. @cjwagner what do you think we should be doing here?

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/configure-prow-url-response branch from 5a86c70 to 37e38c2 Aug 30, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

@munnerz i had to futz with your DCO tests a lot here since you were asserting equality on mangled comments missing org/repo :)

Allow configuring the Prow URL for command help
Not all Prow deployments will use the configurations that prow.k8s.io
does, and it is best to link to the direct help page for the repo in
question for the deployment that is used to run it.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/configure-prow-url-response branch from 37e38c2 to a3e0e08 Aug 30, 2019

test.name,
expect,
got,
diff.StringDiff(expect, got),

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Aug 30, 2019

Author Contributor

Type of error we used to get:

        dco_test.go:901: expected comment to be "org/repo#3:Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.\n\n:memo: **Please follow instructions in the [contributing guide](https://github.com///blob/master/CONTRIBUTING.md) to update your commits with the DCO**\n\nFull details of the Developer Certificate of Origin can be found at [developercertificate.org](https://developercertificate.org/).\n\n**The list of commits missing DCO signoff**:\n\n- [sha](https://github.com///commits/sha) not a sign off\n\n<details>\n\nInstructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](http.com/bot-commands?repo=org%2Frepo).\n</details>\n" but it was "org/repo#3:Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.\n\n:memo: **Please follow instructions in the [contributing guide](https://github.com/org/repo/blob/master/CONTRIBUTING.md) to update your commits with the DCO**\n\nFull details of the Developer Certificate of Origin can be found at [developercertificate.org](https://developercertificate.org/).\n\n**The list of commits missing DCO signoff**:\n\n- [sha](https://github.com/org/repo/commits/sha) not a sign off\n\n<details>\n\nInstructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](http.com/bot-commands?repo=org%2Frepo).\n</details>\n"

(unhelpful :) )

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

@cblecker @spiffxp @nikhita any thoughts?

if _, err := url.ParseRequestURI(o.prowURL); err != nil {
return fmt.Errorf("invalid -prow-url URI: %q", o.prowURL)
if len(o.prowURL) > 0 {
logrus.Warn("--prow-url is deprecated, set prow_url in config.yaml instead")

This comment has been minimized.

Copy link
@cblecker

cblecker Sep 8, 2019

Member

Do we need to go through the deprecation policy for this?

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Sep 9, 2019

Author Contributor

I doubt anyone but me uses refresh but yes, we can default here.

@@ -51,6 +51,9 @@ type Configuration struct {
// Owners contains configuration related to handling OWNERS files.
Owners Owners `json:"owners,omitempty"`

// ProwURL is the URL to use in help messages when referring to this deployment
ProwURL string `json:"prow_url"`

This comment has been minimized.

Copy link
@cblecker

cblecker Sep 8, 2019

Member

Do we need to have two separate settings for this? (one in config.yaml, and another in plugins.yaml?)

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Sep 9, 2019

Author Contributor

Not everyone loads both :(

It's awful, suggestions appreciated here (@cjwagner ?)

This comment has been minimized.

Copy link
@cjwagner

cjwagner Sep 9, 2019

Member

That belongs in Prow config only (not plugin config) IMO. Core Prow config is also exposed to plugins and the Prow URL makes more sense in core config anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.