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

Prevent getJSON failing to retrieve data from setting a non-zero exit code #5201

Closed
kaushalmodi opened this issue Sep 11, 2018 · 8 comments
Closed

Comments

@kaushalmodi
Copy link
Contributor

kaushalmodi commented Sep 11, 2018

This issue is a continuation of #5076.

While #5199 prevented hugo from immediately exiting, the end results are still the same as before because the hugo build step still exits with an error code.

I talk about this in #5199 (comment). Below is copy/paste from there for convenience:


The whole point of that issue I opened was to build sites like normal even if getJSON failed to retrieve anything from the URL.

With the latest master, running hugo gives me:

Building sites … ERROR 2018/09/11 11:12:57 Failed to get JSON resource https://gitlab.com/api/v5/projects/kaushalmodi%2Fkaushalmodi.gitlab.io/repository/commits?path=content-org&ref_name=master: Failed to retrieve remote file: Unauthorized
ERROR 2018/09/11 11:12:58 Failed to get JSON resource https://gitlab.com/api/v4/projects/foo%2Fkaushalmodi.gitlab.io/repository/commits?path=content-org&ref_name=master: Failed to retrieve remote file: Not Found
Total in 1794 ms
Error: Error building site: logged 2 error(s)

This functionally just like before because Makefile/Netlify would not proceed on seeing that error code (even if the public/ gets populated after this commit).

You are free to ignore the exit code if you want.

I cannot also unconditionally mask errors else I will reach the deploy step for real errors too.


Probably making that "Failed to get JSON resource .." ERROR a WARNING fixes this issue? Or not letting getJSON return an error on getJSON failure?

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Sep 11, 2018

I thought, doing this change would work.. but it doesn't.

With this, the build finishes fine, but no warning is printed.. why is that?

diff --git a/tpl/data/data.go b/tpl/data/data.go
index 14a4975a..06fd7f87 100644
--- a/tpl/data/data.go
+++ b/tpl/data/data.go
@@ -111,7 +111,7 @@ func (ns *Namespace) GetJSON(urlParts ...string) (v interface{}, err error) {
 		var c []byte
 		c, err = ns.getResource(req)
 		if err != nil {
-			ns.deps.Log.ERROR.Printf("Failed to get JSON resource %s: %s", url, err)
+			ns.deps.Log.WARN.Printf("Failed to get JSON resource %s: %s", url, err)
 			return nil, nil
 		}

@bep
Copy link
Member

bep commented Sep 11, 2018

You can create a binary wrapper around Hugo that "swallows" the exit code.

So, whether this is an ERROR or not, is contextual. We cannot make it into a WARNING just make you happy. Many people use this getJSON to fetch ... important stuff.

We could consider adding an "ERROR threshold" (i.e. only fail the build if more than n errors).

@kaushalmodi
Copy link
Contributor Author

a binary wrapper around Hugo that "swallows" the exit code.

As I mentioned earlier, I cannot ignore all exit codes. For example, I don't want to ignore error when unmarshalling a JSON in:

		err = json.Unmarshal(c, &v)
		if err != nil {
			ns.deps.Log.WARN.Printf("Cannot read JSON from resource %s: %s", url, err)
			ns.deps.Log.WARN.Printf("Retry #%d for %s and sleeping for %s", i, url, resSleep)
			time.Sleep(resSleep)
			deleteCache(url, ns.deps.Fs.Source, ns.deps.Cfg)
			continue
		}

Many people use this getJSON to fetch ... important stuff.

Right, but with the current state of getJSON returning nil, it shouldn't matter if it's an error or a warning. Anyone using this would then handle the nil-return case too.

We could consider adding an "ERROR threshold" (i.e. only fail the build if more than n errors).

It's not the number of errors.. it's this specific error I'd like to ignore.


I don't mind committing a patch to my personal hugo build.. but on quick testing, replacing ERROR with WARN in the above diff didn't seem to work. With that, I don't get the warning printed at all.

@kaushalmodi
Copy link
Contributor Author

@bep In this commit d1661b8#diff-a13cdf5921982a729b6429e57b95fdfb, you moved away from using the ns.deps.Log.ERROR. ... I was hoping that you will switch that to ns.deps.Log.WARN. .. at some point. But that commit is now doing return nil, _errors.Wrapf ...

Can you consider making that JSON fetch a warning?

@bep
Copy link
Member

bep commented Oct 24, 2018

Is there a practical difference between the earlier ERROR log and the now ... delayed ERROR log?

@joshkadis
Copy link

This issue comes up a lot both here and in the forums. In the many threads that I've read, it seems like every commenter would prefer not to abort the site build when there's a failure in getJSON, whereas @bep takes a position like "An ERROR is an ERROR and should be treated as such." (Excuse me if I've misstated that.) I'm sure there are some folks out there who agree with @bep but either they haven't cared to comment or I've missed their comments.

Both positions have merit. As a compromise, what about using a config variable to decide if a getJSON failure should be handled as an ERROR or a WARNING?

If we want to get fancy, this could even be a regex (e.g. a WARNING for /twitter\.com/) but a boolean would be better than nothing.

Thoughts?

@stale
Copy link

stale bot commented Jun 20, 2019

This issue has been automatically marked as stale because it has not had recent activity. The resources of the Hugo team are limited, and so we are asking for your help.
If this is a bug and you can still reproduce this error on the master branch, please reply with all of the information you have about it in order to keep the issue open.
If this is a feature request, and you feel that it is still relevant and valuable, please tell us why.
This issue will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

@stale stale bot added the Stale label Jun 20, 2019
@stale stale bot closed this as completed Jul 20, 2019
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants