-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
roachtest: move roachtest stress CI job instructions to README #69239
roachtest: move roachtest stress CI job instructions to README #69239
Conversation
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.
Though until the thing actually works should we remove it? I also think it might be better in general to have these docs in a place that's easier to edit and link to that, like the readme for roachtest readme. Your call
There's no other project we could use in the meanwhile?
It's a bit wordier than I'd like (unfortunately there doesn't appear to be a way to link directly to the run form, nor to prepopulate the inputs), but I find these instructions much more useful when they just tell me what to do rather than having to go read something. Not a strongly held opinion though, and I've added a section on this to the |
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'm partial to the inline instructions myself.
Reviewed 1 of 1 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)
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.
Interesting. My detailed pro-link arguments are:
- it means that there is only one source of documentation (so nothing will go stale)
- the email notifications get smaller (I primarily interact with roachtest issues through email, and don't need to see the detailed instructions dozens of times a week but the summary folding doesn't work in email-constrained html)
- going through a PR to make even small tweaks + being constrained in how much I write + dealing with the markdown is painful
- starting a roachtest job will take at least five minutes and isn't too frequent an operation, so adding an extra click to it seems totally justified
I can live with the status quo though.
It's a bit wordier than I'd like (unfortunately there doesn't appear to be a way to link directly to the run form, nor to prepopulate the inputs), but I find these instructions much more useful when they just tell me what to do rather than having to go read something. Not a strongly held opinion though, and I've added a section on this to the README in any case since it seems like something we'd want to mention there as well.
Another thing I've noticed is that it's really annoying to pick the SHA from the TeamCity "Changes" dropdown, unless the SHA is very very recent. I wonder if using the REST API would be a lot easier: https://www.jetbrains.com/help/teamcity/rest/start-and-cancel-builds.html
Would be nice if we didn't have to POST to that endpoint (so you could just do it in your browser, where you're already logged in). With POST there's likely a need to get a token, etc, so there's an upfront setup step. Might still be preferrable, though. The instructions would be: get an API token using these steps. Then run this script and it will ask you for stuff (like roachstress.sh) and then give you the build ID.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)
My reasoning here is that we should optimize for ease of use: when some developer is tasked with investigating a roachtest failure, we want to make that process as hassle-free as possible, which should include making all relevant information easily available and discoverable. I think it's worth sacrificing maintainability and email brevity for that, but if enough users complain I think a link would be fine too.
Yeah, we considered this in the original PR, but given the added hassle of obtaining the auth token we decided to punt it. If we could just run a That said, creating an access token is pretty trivial, so maybe we should just do that instead. I'll open an issue, need to focus on some other tasks first. |
My reasoning here is that we should optimize for ease of use: when some
developer is tasked with investigating a roachtest failure, we want to make
that process as hassle-free as possible, which should include making all
relevant information easily available and discoverable. I think it's worth
sacrificing maintainability and email brevity for that, but if enough users
complain I think a link would be fine too.
What I don't understand is why you don't think the link checks these boxes
more than the inline version. You are tasked to investigate a roachtest.
The issue has a section called "Reproduce" (maybe we can rename it to
"Investigate"). You follow the link and you get a detailed tutorial that is
much better than whatever we can justify squeezing into the issue,
screenshots and all. Are you worried that... people will not click the
link, throw their hands up, and walk away? Folks won't get a roachtest and
immediately try to stress the job. They need to investigate first, right?
We're not going to be able to do that inline. Maybe we're over-indexing a
bit on ourselves, where we mainly need to be reminded of the link to the
build? I do think the average CRDB dev needs a lot more hand holding than
that.
…On Tue, Aug 24, 2021 at 12:30 PM Erik Grinaker ***@***.***> wrote:
#69289 <#69289>
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#69239 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGXPZEDEHU4Z3JWVBUDXLLT6NYCRANCNFSM5CUYV4YQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
0af4559
to
85c146e
Compare
Release justification: non-production code changes Release note: None
85c146e
to
a452dbc
Compare
Sure, removed the instructions. bors r=tbg,stevendanna |
Build failed: |
bors r=tbg,stevendanna |
Build succeeded: |
Release justification: non-production code changes
Release note: None