Handle errors when couldn't download config or compose

[dpordomingo]

fix #245
motivated by #241

If the user initializes or tries to download a config without access to the internet, the error is a bit cryptic.

This PR adds clearer error messages in such situations and also points to some docs explaining why is that assets needed.

---

When there is no local docker-compose installation, and it could not be downloaded:

• because of network error:
  

• because of write error when saving the file:
  

---

When trying to download and activate a new docker-compose.yml config, but the process failed:

• because of network error:
  

• because of write error when saving the file:
  

disclaimer

This PR does not explain how to run sourced offline, which could be done separatelly if needed.

---

• I have updated the CHANGELOG file according to the conventions in keepachangelog.com
• This PR contains changes that do not require a mention in the CHANGELOG file

[se7entyse7en]

I don't know how much I like the usage of defer to just wrap an error. Is this an idiomatic go thing? I mean I'd find the usefulness of this if you want to also recover from panic and maybe also wrap the panic error. But in this case, maybe a simple wrapper function would do the trick?

[dpordomingo]

do you mean something like the following?

func Download(revOrURL RevOrURL) error {
  err := downloadWrappedErr(revOrURL)
  if err != nil {
    return ErrCouldNotGetComposer.Wrap(err)
  }

  return nil
}

func downloadWrappedErr(revOrURL RevOrURL) error {
  // original content of Download(RevOrURL) error
}

It seems ugly imo

On the other hand, if I understood the proposal for try, handling and wrapping errors in defer seems to be something proposed by go team, isn't it?

We advocate using the existing defer statement and standard library functions to help with augmenting or wrapping of errors.

golang/go#32437

Does not seems a clean and explicit way to handle and wrap these errors?

[se7entyse7en]

yup, but that proposal has been closed and rejected due to a lot of criticism AFAIU.

[se7entyse7en]

BTW I was thinking something as follows:

func Download(revOrURL RevOrURL) error {
  wf := func(err) {
    if err != nil {
      return ErrCouldNotGetComposer.Wrap(err)
    }
    return nil
  }

  ...
    return wf(err)
  ...

  return nil
}

[se7entyse7en]

But I'm not a golang expert, so I really cannot strongly advocate some styles over others.

[dpordomingo]

The goal of defer is to ensure that Download returns a ErrConfigDownload, no matter where it's called.

If not used the defer, I see three alternatives:

• a) make sure we wrap the returned error wherever Download is called, as I understood from @smacker Handle errors when couldn't download config or compose #247 (comment),

  // in cmd/sourced/compose/file/file.go
  func InitDefault() (string, error) {
  	...
  	err := Download(version)
  	if err != nil {
  		return "", ErrConfigDownload.Wrap(err)
  	}
  	return activeFilePath, nil
  }
  
  // in cmd
  func (c *composeDownloadCmd) Execute(args []string) error {
  	...
  	err := Download(version)
  	if err != nil {
  		return ErrConfigDownload.Wrap(err)
  	}
  	return nil
  }

  what is error-prone imo, because Download can be called elsewhere tomorrow, so then it could be not wrapped.
• b) make sure that we wrap all the returned errors inside Download, what is repetitive and can be also error-prone if a new return is added later and not wrapped. That's what I understood from @se7entyse7en's Handle errors when couldn't download config or compose #247 (comment)
• c) do as suggested by Handle errors when couldn't download config or compose #247 (comment), moving the current content of Download into a new (and unexported) function which will do the actual work, and then call it from (the exported) Download, and wrap and return its error.
  This would be bulletproof and not error-prone (if the new unexported function is not directly called from the same package elsewhere), but as I said, I find this procedure ugly and could be an anti-pattern.

If you strongly oppose to deferring the error handling, and you don't suggest another option, I'd vote for b as @se7entyse7en suggested, because even being error-prone as I said, the risk is limited to the scope of the same function.

[se7entyse7en]

I also like option c) actually, and I don't think it's anti-pattern. For example have a look at the code of net/http in client.go here. It uses an expored method that only calls an unexported one. In this case there's no error handling, but I don't think would be considered an anti-pattern.

[smacker]

what is error-prone imo, because Download can be called elsewhere tomorrow, so then it could be not wrapped.

you assume that caller from the different location must behave exactly like the current one, which isn't true.
make-up example:

err := Download(url)
// with current code I have to know that `Download` returns `go-error` type
// which I must inspect using `Case()` to make this check
if err == NetworkError {
  return LocalFallbackUpdate()
}
// with current code produces weird text like
// "can't set new config: can't download config: no permission to change active config"
return ErrCanNotSetNewConfig.Wrap(err)

As you can see the error is just incorrect. It could download the config. It failed on SetActive.

[smacker]

@se7entyse7en jfyi we discussed it offline and I found a better explanation which David agreed with. With current state of go 1.12 you usually return errors "one level less". Let me give you an example:

func downloadFile() error {
  // unwrapped
  return err
}

func setComposeFIle() error {
  // unwrapped
  return err
}

func UpdateCompose() error {
  if err := downloadFile(); err != nil {
    return ErrDownload
  }

  if err := setComposeFIle(); err != nil {
    return ErrSetComposeFile
  }
}

You don't need to wrap an error in the function downloadFile as the caller already knows it is download error. That information comes from the name of the function and UpdateCompose function can inspect if it was network or fs error and do something else (like fallback) using only current features of the language without pulling external lib like go-error into the codebase. On another hand UpdateCompose (if it makes sense) wraps error underlying errors to allow the caller distinguish what happened. (without wrap you don't know if fs error was caused by downloading of set)

[se7entyse7en]

Thanks for the explanation! Seems really clear!

[carlosms]

Most people will not need this completely offline use case. And for people behind a firewall, I assume most of the times they will make it work configuring their firewall rules.
I think this section is a bit out of place in the quickstart, and it can be confusing to explain a use case that is not the common one, or supported.

What do you think about moving this to the FAQ? This way we have a place to link if somebody asks, but it's not distracting to first time users.

[dpordomingo]

I'm not strongly against moving this into our FAQ, but I really see the connection as a requirement to be made explicit when the user is installing source{d}.

We both agree that the intended use of source{d} requires Internet, but in the last few weeks, we found a couple of people having this very same problem #206 #241, what I think it's legit because of the target of this tool (being used by engineers to try its features, to be applied in big corps, where there can be firewalls or other net limitations).

On the other side, this doc does not explain how to use the tool without net connectivity, but only list the resources to be requested from the net, that should be satisfied if there is no connectivity available.

From the reader pov, I'd find easy to skip the whole section if they have connectivity, and just read it if they don't (what might be interesting considering what was exposed above).

So, if the goal is to have a shorter quickstart as possible, and you agree on the importance of not considering Internet as a supposed resource, would you find better if we just:

• mention the connectivity requirement in this section, and
• move the extra details about the required resources & what to do if not available to the FAQ.

[carlosms]

My view in general is that for docs, less is more. In a way I think that documenting too much is similar to feature creep in the code. This section is not bad. It's not misleading or confusing. But if we go this direction, things add up, and then it becomes noise in my opinion. Should we also include in the quickstart that you need to make sure the ports we'll try to use are free? storage requirements? Or taking it to the extreme: should we explain custom deployments, like for example how deploy connecting to a postgresql already deployed in your company network?

For the issues you link:
#206 already contains the error Get https://.... docker-compose.yml: Forbidden. The documentation here does not add any extra information that would help to figure out how to configure the firewall.

In #241 the problem was that they could manually provide the file sourced-ce needed, but didn't know where to copy it manually. The improved error messages from this PR would be enough to solve their problem.
In this case this section would also not provide a solution, we say you will need to provide the file manually but don't document where exactly. Also, the docs here link to the docker-compose.yml from master, and that's not correct, it can lead to the deployment not working as expected if the user places this file manually in their ~/.sourced dir.

[dpordomingo]

About #206, when they tried to initialize sourced init local .
before:

Get https://raw.githubusercontent.com/src-d/sourced-ce/v0.14.0-rc.2/docker-compose.yml: Forbidden

The user doesn't know why is that file needed, and how to fix it to be used without internet (because it is not said to be a requirement)

now:

[...]: error downloading https://raw.githubusercontent.com/src-d/sourced-ce/master/docker-compose.yml to /.sourced/compose-files/master/docker-compose.yml: [...]

The source{d} CE config file could not be downloaded. see docs about internet requirement.

The error message now explains which kind of file is failing and points to docs explaining that internet connection is a requirement.
The error also explains where should be placed the file (which could be used by a ninja engineer to provide that file)

As agreed, the purpose of the docs is not to explain how to run source{d} offline, but at least explain clearly what and why is failing.
What we had before, already let the user know that a file could not be downloaded, but it seems it was not enough, because they felt the necessity of opening an issue, I guess if it was because they didn't assume that source{d} was not meant to be run offline.

About #241 it's kind of the same. It was needed three interactions to realize that the problem was a lack of connectivity, so I'd say that this assumption might be too much for our users.
For sure that if we start having questions/problems about port conflicts, we'll have to document or provide an alternative, but we can manage that problem when it happens.

[carlosms]

As agreed, the purpose of the docs is not to explain how to run source{d} offline, but at least explain clearly what and why is failing.

But the docs here don't explain what and why is failing. The error contains more info that the docs: trying to access the internet, the exact URL, the destination file, and the error.
For #206 the docs don't mention firewalls or Forbidden errors, or the destination path for the file. Even the URL is wrong, it points to master when it will be http...<version>/docker-compose.yml.

About #241 it's kind of the same. It was needed three interactions to realize that the problem was a lack of connectivity, so I'd say that this assumption might be too much for our users.

Of course they knew they needed connectivity to download a file! The problem was not that, the problem was that they didn't know where to place the file that was provided manually:

Edit: Also, I have in my notes that he tried fetching run.sh separately, but could not figure out where to put it, so that it would work correctly with the other files.

In this case, the new errors from this PR would solve the problem. But the docs don't mention the URL or destination path for the missing file, so it would not help in this case either.

For sure that if we start having questions/problems about port conflicts, we'll have to document or provide an alternative, but we can manage that problem when it happens.

I was actually arguing for the exact opposite... to not document every possible problem. sourced init is already capable of telling the users when the containers cannot be created due to a port conflict.

[carlosms]

Similar to the other comment, I think for most people it can be confusing to explain how to run things offline, when that is not the intended use case.
I'd prefer to remove the link to the docs, the error message should be enough to let the user understand that they need to connect to the internet, open their firewall, or provide the file manually.

[dpordomingo]

The linked docs do not explain how to run source{d} offline. They only explain that docker-compose is a requirement, and how can it be satisfied.
I think that pointing to docs from errors can be a good idea.

[smacker]

My 5 cents. I agree with Carlos to put instruction on how to run it without the Internet into FAQ for the same reasons he explained above.
Plus people tend to skip reading the documentation if there is too much text (I'm a good example of this) so keeping only what is really important increases chances that it will be actually read.

[dpordomingo]

Thanks for your deep review.
I rebased on top of the current master, and apply the changes you suggested:

• moved docs into FAQ
• avoid defer
• separate errors caused by the network from the ones caused when the config could not be written.

[dpordomingo]

Inline suggestions applied, thanks @carlosms !
@se7entyse7en your suggestions about removing defer were addressed following the conversation I had with @smacker on last Friday. Does it suit you?