43

I was invited to review some code. I've never done that before and barely know the project. Thus I'm following the README step by step. I log everything.

The README says in its "Quickstart" section:

docker-compose up -d

This line raised "..denied..access forbidden.." which lead me to invest some reasonable time into researching it. A friend later told me to add build: . and remove image: docker.git... from the docker-compose.yml. This fixed it.

I'm tempted to change the README accordingly, though I hesitate as maybe I'm simply lacking the skills my role demands.

https://readmetips.github.io/#essential-tips-for-a-nice-readme

Improve this question
9
  • 93
    In other words "The README doesn't work. Should I fix it?"
    – user253751
    Aug 16, 2021 at 15:05
  • 23
    The cause for the error message is most likely that you are not authenticated against the docker repository specified, and the hint only works around it. In my opinion you should ask your experienced colleagues what that error means, and have them tell you how to fix it, and then add that information to the README file.
    – Thorbjørn Ravn Andersen
    Aug 16, 2021 at 23:25
  • 9
    If you're editing the project files to get things to work, that's mostly not "a rookie hint"; it's a sign that the project is broken, it's incompatible with some part of your system or you're trying to run it incorrectly. Although you might consider it a "hint" until it can be properly fixed.
    – Bernhard Barker
    Aug 16, 2021 at 23:39
  • 2
    But the README looks correct here, it's the underlying code (docker-compose.yml) which is wrong
    – usr-local-ΕΨΗΕΛΩΝ
    Aug 17, 2021 at 12:35
  • 2
    This is very typical of documentation for Docker-related things, in my experience. Either they provide wrong information, or they depend on things being set up in a different way than they really are - but without telling you. This plagues documentation in the Docker world in general, and the people writing these things need to get away from an "It works on my machine" mentality.
    – Panzercrisis
    Aug 19, 2021 at 14:56

7 Answers 7

Reset to default
90

If you think of this from a business perspective: the more time developers (whether they are new or experienced) need to find things out, the more expensive it is for the client.

So: The more information in the README to help people get up and running quickly, the better and cheaper for the client.

In other words: By all means add information to the README to help people get it working without wasting time and money.

Improve this answer
8
  • Correct. Information that is there, but not needed, can be easily ignored (assuming appropriate formatting). Information that is missing, but needed, cannot be as easily found.
    – Cody Gray - on strike
    Aug 17, 2021 at 0:14
  • 6
    There are though some caveats. Longer Readmes and Wikis may be harder to maintain. If something changes then you may change it in Readme, possibly in many places. If you do not maintain the Readme it may become stale and misleading. Also longer Readme may take more time to actually read.
    – Gherman
    Aug 17, 2021 at 12:07
  • 13
    "The more information in the README to help people get up and running quickly, the better and cheaper for the client." On the other hand, readmes that try to account for every possible thing under the sun become cumbersome and readers constantly lose momentum by being sidetracked. There is a reasonable limit here to how much tangential information a document should concern itself with.
    – Flater
    Aug 17, 2021 at 12:31
  • 2
    @Jesper I read your answer as an endorsement of adding an unintended workaround not solving the underlying problem to the README. I agree that correct information helping the reader should go there. In this case, it might be a prerequisite - "You must be able to X, Y and log into the docker repository so docker .... works"?
    – Thorbjørn Ravn Andersen
    Aug 18, 2021 at 7:05
  • 15
    @Flatter, you can always add a "Common Issues" or "Troubleshooting" appendix which does not burden the main step-by-step portion of the README with information that's used less often.
    – dbkk
    Aug 18, 2021 at 12:31
94

Code reviews by people who are less familiar with the project are extremely valuable precisely because they discover such issues. “Well, obviously you need to do this and that first” says the developer who had been doing nothing but this project for the last six month, but it isn't obvious to other people.

So yes, adding an actually working setup guide could be really valuable. You're not doing this for you; you're doing this for the next person who will have to deal with this project.

Such problems with things that should work (but don't) could also indicate a flaw with how the project is configured. For example, the real problem could be one of the following:

  • There is no continuous integration style testing system that checks that the project can be deployed from scratch.
  • The project expects that you manually docker build the image locally and tag it before the docker-compose file can be used. But why are there manual steps? Scripts are executable knowledge.
  • The docker-compose file expects that you're logged in to a private container registry, but you weren't given the necessary credentials.
  • There is an error in the docker-compose configuration and it should always build instead of using an existing image. This might have even hidden bugs during initial development!

None of these issues reflect badly on you.

Improve this answer
1
  • In addition to your first bullet, CI also discovers whether something only works for the original author / team member / someone with privileges, versus anyone who'd like to contribute.
    – JBRWilkinson
    Aug 19, 2021 at 14:32
35

I'll take a different stance here. You write:

A friend later told me to add build: . and remove image: docker.git... from the docker-compose.yml. This fixed it.

I apologize if I misinterpret what you wrote, but, to me, this sounds like you don't actually understand why this fixed your problem.

There's a rule in professional software development, and it applies to documentation as well: Don't write code you don't understand. As Karl Bielefeld wrote in his answer, there might be a better fix. Or, worse: this might not be a fix at all. It might just hide the underlying problem and make it seem like everything works.

Contrary to Karl's answer, I think you should not propose the change and wait for any mistakes to be caught in a review. Instead, find out why this fixed the problem. This might include learning more about Docker, asking the people who wrote the script and/or asking your friend for more details. That's OK. Software engineering is not about typing. It's all about thinking and learning new stuff.

Only after you found the underlying cause, think about the best way to fix it.

Improve this answer
6
  • 11
    Also, why is the docker-compose file written the way it is? Seems to me that the thing to change is that file and not the readme file.
    – Taemyr
    Aug 17, 2021 at 9:10
  • 1
    @PeterMortensen: I've been reading and writing English for years (actually: decades, now that I think about it), and only today I learned that the "comma splice" is actually considered incorrect. Thanks for the edit and the link!
    – Heinzi
    Aug 18, 2021 at 6:27
  • 1
    I must object here. This may be the right thing to do, but in this particular case it is much more likely that the OP would be side-tracked into an issue they aren't in position to fix. The reasonable middle ground is to describe the situation explicitly in the readme, like this: You may encounter error 'missing frobnicator'. I'm not sure what is the underlying issue, but running 'frobnicator --configure' fixed the issue for me.
    – Frax
    Aug 18, 2021 at 11:23
  • @Heinzi You were right originally, nothing wrong with comma splices.
    – curiousdannii
    Aug 18, 2021 at 13:30
  • 2
    Wish I could upvote x10! In other words, please understand the problem and the solution. Please avoid arbitrary workarounds. Whether you (OP) should solve the problem I leave for them to decide. Please either understand what you are doing, or clearly write "I don't know why, but this worked for me".
    – Pablo H
    Aug 18, 2021 at 14:19
20

Remember I can read faster than you can write. Unless you belabor the point or cause distracting confusion I can easily skip over what I find obvious.

When documenting a project the worst people to judge the documents are the ones working on the project. This is because of something called the curse of knowledge. You already know how much of this works so you feel dumb mentioning obvious details. But you can't be trusted to know what is obvious.

This is why my policy on every project is to have the newest member of the team review everyone's documentation. This isn't how we haze the new guy. This is because I don't trust myself to understand how our documentation looks to normal people. It's also work I volunteer for when I'm the newbie.

That aside, please document all your useful tricks. But take the time it takes to document well so it's easy to skip over the stuff that is obvious to me. If every time you work on the documentation it's getting longer then you aren't there yet. Towards the end it should be getting shorter as you organize better and edit out the needless fluff.

Improve this answer
2
  • 2
    +1 The first 'task' that I give a new team member... is to update the onboarding documentation. Not only will their perspectiuve be invaluable,. but also, over time, procedures change and not everyone remembers or is motivated to update the README to reflect those changes
    – Michael Durrant
    Aug 17, 2021 at 12:01
  • +1 from me too, but distraction isn't the only potential problem. It's good to write docs with not just readability in mind, but also maintainability. For example, if the rookie hint is for something outside of your control, like how to install Docker on Windows, a well-placed link to an official tutorial would be more maintainable than explaining the solution yourself.
    – Maxpm
    Aug 20, 2021 at 17:45
14

Presumably your documentation changes get reviewed by others, so there should be no harm in proposing it. Having the right mindset to spot this sort of problem is likely one of the reasons you were asked to review it.

However, keep in mind there may be a better fix than the one your friend proposed. Maybe it should be changed to build for everyone. Maybe there is some sort of step to set up credentials that should be added. Maybe the listed image is out of date or has been moved. Maybe there's something else about your environment that needs changing. If you propose your documentation change, hopefully people will point out these sorts of things when your change is reviewed.

Improve this answer
3

It is definitely an issue that should be fixed. The question is who should fix it?

Usually who does the code review writes comments about what needs to be fixed, they don't fix things themselves. One reason for this is that the developer should learn what they did wrong in order to avoid doing the same mistake again, but also for other reasons. In this case, a configuration change is something delicate, and these things often end up in the works on my machine situation. So even a small change in the README should be verified by someone who knows the project very well.

If you want to fix the issue by yourself because the developer is no longer there, or on holidays, you can go on with the change, but first you should check if there is someone around who worked on the project or a system administrator who deployed it often and ask for a confirmation.

Improve this answer
1
  • Exactly this. Your review comment could be "I expected the Readme to tell me X, Y and Z". Then the developer who knows the subject matter can fix up the Readme. It's certainly not your job as reviewer to do that.
    – Dawood ibn Kareem
    Aug 18, 2021 at 22:54
2

The maintainer of the project or the writer of the original code is the best person to fix bugs you found during your code review.

If I'm invited to review some code, I do exactly that: a review. I usually don't implement changes to the code myself, unless they are trivial or obvious changes (for instance a typo)

If I'm unsure if I stumbled across a bug, I provide comments to the code and explain my concerns, and let the maintainer decide if it has to be solved and how.

That would be my advice to you in this case, as you're unsure if this is an mistake in the docs or if your local setup is broken.

Improve this answer
1
  • The best person to tell whether to make a change or not, is the person who tasked the OP with this work. I personally would not worry about the end-user not being able to do a build via docker-compose because I know that the target audience will not get denied..access forbidden. I would tell as much to my reviewer if they asked me that. So the best thing to do is ask the actual person, not strangers on the internet who know nothing about the intentions. I think this answer is the best answer so far.
    – Andrew Savinykh
    Aug 19, 2021 at 7:44

Not the answer you're looking for? Browse other questions tagged or ask your own question.