Skip to content

Comments

Issue 2755 - Troubleshooting PRs#2854

Merged
knative-prow-robot merged 10 commits intoknative:masterfrom
mpetason:issue-2755
Jan 28, 2021
Merged

Issue 2755 - Troubleshooting PRs#2854
knative-prow-robot merged 10 commits intoknative:masterfrom
mpetason:issue-2755

Conversation

@mpetason
Copy link
Contributor

Fixes #2755

Proposed Changes

  • Added a troubleshooting section with basic information

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 24, 2020
@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 24, 2020
@mpetason
Copy link
Contributor Author

Looking for feedback on some of the sections. I added some basic information but am not an expert on each section.

@abrennan89 abrennan89 requested review from abrennan89 and mattmoor and removed request for samodell September 25, 2020 13:39
@abrennan89
Copy link
Contributor

To fix #1 either you need to force push your PR or someone @ Google has to manually add the label I think, so I'd add that maybe.
Thanks @mpetason !

CONTRIBUTING.md Outdated

### Common Troubleshooting issues for PRs

1. CLA fails even though you have signed a CLA. This can happen if someone accepts and commits suggestions due to the email address not matching the address on record for the CLA.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. CLA fails even though you have signed a CLA. This can happen if someone accepts and commits suggestions due to the email address not matching the address on record for the CLA.
1. The CLA check fails even though you have signed the CLA. This may occur if you accept and commit suggestions in a pull request from another person's account, because the email address of that account doesn't match the address on record for the CLA.

Copy link
Member

@mattmoor mattmoor Sep 25, 2020

Choose a reason for hiding this comment

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

IIUC this is generally because Github shows that commit as co-authored by the two parties (author and suggester) and uses their primary github email address as the commit email.

I'd love it if CLA bot didn't blow up 9/10 times on suggested edits. It's honestly turned the feature from something I missed from Google's Critique to something I hardly think folks should use at all (cc @thisisnotapril , not sure what kinda juice you have with the team 😉 )

CONTRIBUTING.md Outdated

1. CLA fails even though you have signed a CLA. This can happen if someone accepts and commits suggestions due to the email address not matching the address on record for the CLA.

1. Test Fails. If you do not see a specific error related to a change you made, and instead the errors are related to timeouts then you may want to rerun the test at a later time. There are running tasks that could result in timeouts or rate limiting if your test runs at the same time as the tasks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. Test Fails. If you do not see a specific error related to a change you made, and instead the errors are related to timeouts then you may want to rerun the test at a later time. There are running tasks that could result in timeouts or rate limiting if your test runs at the same time as the tasks.
1. One or more tests are failing. If you do not see a specific error related to a change you made, and instead the errors are related to timeouts, try rerunning the test at a later time. There are running tasks that could result in timeouts or rate limiting if your test runs at the same time.

CONTRIBUTING.md Outdated

1. Test Fails. If you do not see a specific error related to a change you made, and instead the errors are related to timeouts then you may want to rerun the test at a later time. There are running tasks that could result in timeouts or rate limiting if your test runs at the same time as the tasks.

1. Previews are not working - WIP (not sure what's causing this)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like stagebot may be fixed now @mattmoor ? It has been working for me lately.

Copy link
Member

Choose a reason for hiding this comment

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

It only runs when the /approve bit is flipped, which makes it seem somewhat unreliable.

This is basically the only piece of knobots that IDK how to turn into github actions, and so I've kinda been hoping a good alternative would present itself. Perhaps we could even get proper netlify preview on this repo (like on website)? @evankanderson @RichieEscarez would know more.

CONTRIBUTING.md Outdated

1. Previews are not working - WIP (not sure what's causing this)

1. Cherrypicking - WIP (not an expert on this)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't really belong in TS maybe, I don't think it's something we have any automation for - if we don't, could we add it based on labels @evankanderson @mattmoor ? I don't know how to do this yet really, just manual CPs, which like I said doesn't really go here in TS?

Copy link
Member

Choose a reason for hiding this comment

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

Automation would be really useful and probably fun to write here.

cc @n3wscott

Copy link
Contributor

@abrennan89 abrennan89 left a comment

Choose a reason for hiding this comment

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

Some suggestions but otherwise looks good 👍 can't think of anything else to add but I'd imagine this doc will expand over time if we find any other issues anyway

CONTRIBUTING.md Outdated
channel](https://knative.slack.com/) to get input on your ideas or find areas to
contribute before creating a PR.

### Common Troubleshooting issues for PRs
Copy link
Member

Choose a reason for hiding this comment

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

I'd think DEVELOPMENT.md is probably a good place for this. I don't feel super strongly, but my spidey sense seems to think this file is largely boilerplate across the repos (I think some may even redirect folks to a common one in community?)...

Copy link
Contributor Author

@mpetason mpetason Sep 30, 2020

Choose a reason for hiding this comment

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

Sorry - thought this was the last part "other issue", I'll take a look and see if we should move it over from contrib to development

@mpetason
Copy link
Contributor Author

@abrennan89 It looks like @mattmoor is suggesting that we move this to Devleopment.md. Maybe we should copy one from the other projects(serving) and update it based on our needs? I actually reference the serving dev guide whenever I setup an environment for knative projects.

@abrennan89
Copy link
Contributor

@mpetason I'm trying to leave @RichieEscarez alone and not tag him for reviews, but I think this one would be better suited for himself than me 😄 he has more expertise in this stuff.

@mpetason
Copy link
Contributor Author

I'll add @evankanderson too since he works across multiple projects and may have some suggestions.

@knative-prow-robot knative-prow-robot added 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 Oct 6, 2020
@mpetason mpetason changed the title [WIP] Issue 2755 - Troubleshooting PRs Issue 2755 - Troubleshooting PRs Oct 28, 2020
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2020

### Common Troubleshooting issues for PRs

1. The CLA check fails even though you have signed the CLA. This may occur if you accept and commit suggestions in a pull request from another person's account, because the email address of that account doesn't match the address on record for the CLA. The commit will show up as co-authored, which can cause issues if your public email address has not signed the CLA.
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't say how to fix it though? I think force pushing fixes it so maybe add that to the doc if the CLA stuff isn't going to be fixed for the time being? cc @thisisnotapril @mattmoor


1. One or more tests are failing. If you do not see a specific error related to a change you made, and instead the errors are related to timeouts, try rerunning the test at a later time. There are running tasks that could result in timeouts or rate limiting if your test runs at the same time.

1. Other Issues/Unsure - reach out in the #docs slack channel and someone will be happy to help out.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. Other Issues/Unsure - reach out in the #docs slack channel and someone will be happy to help out.
1. Other Issues/Unsure - reach out in the #docs Slack channel and someone will be happy to help out.

Copy link
Contributor

@abrennan89 abrennan89 left a comment

Choose a reason for hiding this comment

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

Approving since this looks good as a first iteration. I'd get an LGTM from @evankanderson or @RichieEscarez though ;) Thanks Mike!

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrennan89, mpetason

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@abrennan89
Copy link
Contributor

Since Mike probably won't be around to do more work on this, I'm going to go ahead and merge what we have. We can make updates or iterations later, but this is a good start.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2021
@abrennan89 abrennan89 added this to the v0.21.0 milestone Jan 28, 2021
@knative-prow-robot knative-prow-robot merged commit c1ac45f into knative:master Jan 28, 2021
RichieEscarez pushed a commit to RichieEscarez/docs that referenced this pull request Mar 6, 2021
* Started work on updating contributing docs for different troubleshooting issues

* Updated formatting

* added section to default to reaching out in docs if you have additional issues

* Updated based on reviews

* Removed trailing spaces

* basic edit of development.md to match docs - still wip

* added newline to end of file

* Removed serving reference and updated based on reviews/comments

* Removed empty sections

* Cleaned up trailing whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. 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.

Add troubleshooting information to contributor docs

5 participants