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

Deprecate gubernator! #11302

Merged
merged 2 commits into from
Feb 27, 2019
Merged

Conversation

Katharine
Copy link
Member

This PR changes prow and testgrid configuration to point at Prow, and adds a message to the top of Spyglass pages pointing to Gubernator.

This cannot merge before #11274, #11299, #11292 (or similar), #11207, and #11183 have been actually deployed. Most have merged, but are blocked on our ongoing inability to deploy prow.

/assign @BenTheElder @fejta @spiffxp @krzyzacy @cjwagner
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2019
@k8s-ci-robot k8s-ci-robot added area/prow Issues or PRs related to prow area/testgrid sig/testing Categorizes an issue or PR as relevant to SIG Testing. 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. labels Feb 14, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 14, 2019
prow/config.yaml Outdated Show resolved Hide resolved
@oomichi
Copy link
Member

oomichi commented Feb 14, 2019

/cc @oomichi

@k8s-ci-robot k8s-ci-robot added area/config Issues or PRs related to code in /config size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 14, 2019
@cblecker
Copy link
Member

Will gubernator still be available, but just not be the default?

/hold
Explicit hold as this is a contributor impacting change, and needs to go through the notification/consensus process when it's ready to go.

@Katharine
Copy link
Member Author

Katharine commented Feb 14, 2019

@cblecker Gubernator will still be available for the forseeable future (though presumably not indefinitely). Spyglass pages will have a banner at the top with a link pointing to the corresponding Gubernator page, something like this:

(the exact wording used is defined in this PR.)

@cblecker
Copy link
Member

@Katharine that looks great! No objections from me.

When we're ready to go with this:

  • We need to send an e-mail to testing, contribex, and k-dev outlining the change to experience, what folks can expect, and where to send feedback.
  • At least 72 hours notice required, more if over a weekend, for feedback/lazy consensus.
  • Then :shipit: !

Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

@@ -42,7 +42,7 @@ import (

var configPath = flag.String("config", "../../../prow/config.yaml", "Path to prow config")
var jobConfigPath = flag.String("job-config", "../../jobs", "Path to prow job config")
var gubernatorPath = flag.String("gubernator-path", "https://gubernator.k8s.io", "Path to linked gubernator")
var deckPath = flag.String("deck-path", "https://prow.k8s.io", "Path to deck")
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this a flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

¯\_(ツ)_/¯

Copy link
Member

Choose a reason for hiding this comment

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

@fejta I'm kinda using this test for gerrit jobs as well so...

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2019
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 23a86380af1d2b72910261b195910e014506f321

@Katharine
Copy link
Member Author

Katharine commented Feb 15, 2019

At this point our primary blocker is the fact that we are still unable to deploy Prow. #11292 is only blocked on #11318, and that's the last dependency. Once we finally manage to deploy prow (hopefully by Tuesday?), I intend to send roughly this email:

Hi all!

We have been working on a replacement for Gubernator, which you may know as the page that the "Details" links on GitHub point at.

This replacement, called Spyglass, is now ready for prime time, and we would like to switch all the links to Gubernator to instead be links to Spyglass.

From a user perspective, Spyglass should provide the functionality of Gubernator, but with a hopefully less confusing UI. However, we will keep running Gubernator for the forseeable future, with a link to the corresponding Gubernator page at the top of each Spyglass page.

Spyglass is part of Prow, and enables us to avoid maintaining an extra service for job viewing. It is also designed to be extensible, and in particular open the door for artifact- and job-specific views.

You can try out Spyglass today by clicking the "View in Spyglass" link at the top of Gubernator result pages. Please send general feedback to #sig-testing on Slack.

Do note that this does not affect the PR dashboard at https://gubernator.k8s.io/pr, which we have no current plans to replace and will continue to be supported.

In the absence of any objections, I intend to switch this over for jobs starting on Friday.

Thanks!
- Katharine

@Katharine
Copy link
Member Author

All the dependencies are in. Once we actually manage to deploy Prow (today? maybe? hopefully?) I will send that email.

@Katharine
Copy link
Member Author

Katharine commented Feb 20, 2019

The email has been sent out.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2019
@Katharine
Copy link
Member Author

Nobody has objected during the lazy consensus period, so this is good to merge in principle.

Some bugs were reported and have since been fixed, but not yet deployed. In particular, until #11463 and #11466 have merged and been deployed, we probably don't want to merge this.

/hold

@spiffxp
Copy link
Member

spiffxp commented Feb 25, 2019

explicit +1

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Katharine

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

@stevekuznetsov
Copy link
Contributor

@Katharine will my Deck point to My Gubernator for this change?

prow/config.yaml Outdated
@@ -37,6 +36,7 @@ deck:
- "buildlog"
"artifacts/junit.*\\.xml":
- "junit"
announcement: "The old job viewer, Gubernator, has been deprecated in favour of this page, Spyglass.{{if .ArtifactPath}}For now, the old page is <a href='https://gubernator.k8s.io/build/{{.ArtifactPath}}'>still available</a>.{{end}} Please send feedback to sig-testing."
Copy link
Contributor

Choose a reason for hiding this comment

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

Steve you'll need to set this config value to have it link back to your gubernator

@spiffxp
Copy link
Member

spiffxp commented Feb 27, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2019
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 69091651db1e3c15714e443a980a5b9261554754

@Katharine
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2019
@k8s-ci-robot k8s-ci-robot merged commit c982004 into kubernetes:master Feb 27, 2019
@k8s-ci-robot
Copy link
Contributor

@Katharine: Updated the config configmap in namespace default using the following files:

  • key config.yaml using file prow/config.yaml

In response to this:

This PR changes prow and testgrid configuration to point at Prow, and adds a message to the top of Spyglass pages pointing to Gubernator.

This cannot merge before #11274, #11299, #11292 (or similar), #11207, and #11183 have been actually deployed. Most have merged, but are blocked on our ongoing inability to deploy prow.

/assign @BenTheElder @fejta @spiffxp @krzyzacy @cjwagner
/hold

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.

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. area/config Issues or PRs related to code in /config area/prow Issues or PRs related to prow area/testgrid area/triage 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants