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

Revise troubleshooting guidance #434

Merged
merged 4 commits into from
Nov 18, 2020
Merged

Revise troubleshooting guidance #434

merged 4 commits into from
Nov 18, 2020

Conversation

benthorner
Copy link
Contributor

https://trello.com/c/i3Y2vAua/188-documentation-on-how-to-troubleshoot-apps-failing-to-start-in-govuk-docker

This PR is a review of the general troubleshooting guidance
for apps, which we know is unhelpful for some people.

Please see the commits for more details.

Ben Thorner added 4 commits November 17, 2020 16:55
Previously these encouraged needless re-"make"-ing and "rm"-ing of
containers. Two of the specific problems we have experienced are:

- People not realising *all* commansds need to be run in a container
- People not realising a problem is due to a failed dependency

This rewrites the general app troubleshooting steps to advocate a
more thoughtful approach to debugging - vs. "nuke everything"!
Previously this was quite vague, as it was unclear how "seeing what
is changing" would help someone debug.
Previously I added these thinking it would help organise the guide,
but with only one unique (and unlikely) section on installation, I
don't think it adds any value right now.
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Have had a read and have a couple of suggestions below.

An aside: one note I made from yours & Huw's talk the other week was govuk-docker-up app-live for looking at live data. That may not be the exact command or purpose, but it was new to me and I wrote it down - can't see any further detail about it in the repo though. Is it documented somewhere? :)

docs/troubleshooting.md Show resolved Hide resolved

# try clearing all containers / volumes
govuk-docker rm -sv
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you want to encourage people to diagnose and fix errors 'properly', but sometimes if you're in a bind and need to get something working quickly, this is just the easiest option. I'd really like you to leave it in, even if it's caveated with "You really shouldn't have to do this...!"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree there is a need to have a clean slate. The govuk-docker down command I've suggested is actually equivalent to this, but has the advantage of being easier to explain and write down.

Does that help address your concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, thanks for explaining.

@benthorner
Copy link
Contributor Author

Thanks for the review @ChrisBAshton. I think I may have addressed your comments with a bit more explanation, but let me know if you'd like me to clarify the commits, for example.

The app-live stack has a slightly awkward history, because I think some devs still rely on the startup.sh scripts in each repo, which replicate the functionality. Anyway, the functionality in GOV.UK Docker is documented in the README, and should also now be a little more prominent, since I rewrote it.

Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@benthorner benthorner merged commit b2fb90d into master Nov 18, 2020
@benthorner benthorner deleted the rewrit-troubleshoot branch November 18, 2020 13:35
ChrisBAshton added a commit that referenced this pull request Nov 28, 2024
We've [discussed adding this before](#434 (review)), and shied away from it in favour of more focussed troubleshooting. But sometimes folks just want a quick and easy task to reset things - so let's add this in.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants