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

Peter Mortensen
  • 1,045
  • 2
  • 12
  • 14
jjk
  • 545
  • 93
    In other words "The README doesn't work. Should I fix it?" – user253751 Aug 16 '21 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 '21 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 '21 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 '21 at 12:35
  • If you are not sure why the instruction and compose file do not fit together and ask if you should propose a change to the README in terms of missing cd/preparation commands, patch the compose file - or - rather unlikely document in the README how to tweak the file. – eckes Aug 18 '21 at 03:27
  • Technically you don't need to remove the image: to add a build: - the build: will publish to the tag defined in image: if present. Not on topic for this forum - but I wouldn't what your documentation to be more wrong – Boris the Spider Aug 19 '21 at 05:34
  • 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 '21 at 14:56
  • Was the README part of the code you're reviewing or did you just set the project up to test the change when you otherwise had no affiliation with it / hadn't done it before?

    I would be utterly annoyed as a project lead if somebody changed such an integral line in order to "make it work". At least talk to the team / a senior developer about whether you should not use the image anymore.

     – YetiCGN Aug 19 '21 at 15:41
  • 1
    @Panzercrisis my practice is to include a "install.sh" script that configures the host, even if it is done in a rude and inappropriate manner (e.g. for a dedicated machine) it gives you the context. But this is not a universal practice. – crasic Aug 19 '21 at 19:20

7 Answers7

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.

Peter Mortensen
  • 1,045
  • 2
  • 12
  • 14
amon
  • 134,135
  • 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 '21 at 14:32
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.

Jesper
  • 2,579
  • 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 '21 at 00: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 '21 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 '21 at 12:31
  • I agree with @Flater. This is not a problem directly relevant to the problem, but an incorrectly configured prerequisite and therefore probably irrelevant to a non-rookie. Just as information on installing Linux or Windows would probably not go in this README as well. – Thorbjørn Ravn Andersen Aug 17 '21 at 15:10
  • @ThorbjørnRavnAndersen Of course there is always some practical limit. In a recipe for an apple pie, you also wouldn't explain how to invent the universe first (to mangle a quote from Carl Sagan). Start with a software developer in mind who has the right level of knowledge to write software, but who is unfamiliar with the particular project. – Jesper Aug 18 '21 at 06:58
  • 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 '21 at 07: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 '21 at 12:31
  • pro-tip: Sometimes a naieve change in a pull request to a repository is a really good way to get a conversation about the right fix rolling. "Playing dumb" and proposing a brute-force fix, commented in a way that brings it to the attention of reviewers, can be a better starting point for design conversation than a complicated fix, because it cleanly illustrates the original problem. This also applies to documentation. – Iron Gremlin Aug 19 '21 at 15:31
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.

Peter Mortensen
  • 1,045
  • 2
  • 12
  • 14
Heinzi
  • 9,748
  • 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 '21 at 09: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 '21 at 06: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 '21 at 11:23
  • @Heinzi You were right originally, nothing wrong with comma splices. – curiousdannii Aug 18 '21 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 '21 at 14:19
  • I think this is the wrong attitude. A new developer on a project is almost guaranteed to not understand the project, and nothing is more demoralising than being stuck with sloppy documentation full of obscure references. The one who writes the code needs to literate enough to make their documentation readable; as Einstein once allegedly said: If you can't explain it so a layperson understands, then you don't understand it well enough yourself. – j4nd3r53n Aug 19 '21 at 10:10
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.

candied_orange
  • 108,538
  • 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 '21 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 '21 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.

Karl Bielefeldt
  • 147,435
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.

FluidCode
  • 750
  • 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 '21 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.

Mago
  • 29
  • 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 '21 at 07:44