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

fetch script: unclear error message with invalid GITHUB_TOKEN #38

Closed
verdammelt opened this issue Oct 10, 2020 · 7 comments
Closed

fetch script: unclear error message with invalid GITHUB_TOKEN #38

verdammelt opened this issue Oct 10, 2020 · 7 comments
Labels
status: duplicate This is tracked elsewhere

Comments

@verdammelt
Copy link
Member

The script was not downloading the binary and exiting with non-zero exit code. Some quick debuging showed that $URL was an empty string. Then I checked what curl --header "$HEADER" -s "$LATEST" (as on line 38) output and saw:

{
  "message": "Bad credentials",
  "documentation_url": "https://docs.github.com/rest"
}

Removing --header "$HEADER" fixes the immediate issue and now I have a binary.

However before putting in a PR for this I'd like to know if this is an actual fix or just a workaround and tha the real problem may be elsewhere.

@ee7
Copy link
Member

ee7 commented Oct 10, 2020

The scripts/fetch-canonical_data_syncer in this repo is identical to the configlet repo's scripts/fetch-configlet, apart from the renames.

Here's the diff:

< readonly LATEST='https://api.github.com/repos/exercism/configlet/releases/latest'
> readonly LATEST='https://api.github.com/repos/exercism/canonical-data-syncer/releases/latest'

< FILENAME="configlet-${OS}-${ARCH}.${EXT}"
> FILENAME="canonical_data_syncer-${OS}-${ARCH}.${EXT}"

<         curl --header "$HEADER" -s --location "$URL" -o bin/latest-configlet.zip
<         unzip bin/latest-configlet.zip -d bin/
<         rm bin/latest-configlet.zip
>         curl --header "$HEADER" -s --location "$URL" -o bin/latest-canonical_data_syncer.zip
>         unzip bin/latest-canonical_data_syncer.zip -d bin/
>         rm bin/latest-canonical_data_syncer.zip

So if there's something wrong here then we should change fetch-configlet too.

Just for any others reading, here are the relevant lines:
https://github.com/exercism/canonical-data-syncer/blob/6e162f70b6944e87ffc2ae60314c38f06459541d/scripts/fetch-canonical_data_syncer#L28-L41

I've tested that the script works for me without GITHUB_TOKEN defined. And

curl --header '' -s https://api.github.com/repos/exercism/canonical-data-syncer/releases/latest

seems to work for me too.

See #174 for the PR that added the --header code. It was added because the script was often failing during CI due to rate-limiting issues.

Perhaps you have a bad GITHUB_TOKEN defined?

If not, maybe:

  1. GitHub doesn't like us sending an empty header for unauthenticated requests.
  2. There's something wrong with the awk code.
  3. You're just seeing GitHub blocking you from downloading it sometimes because the request is unauthenticated.

@verdammelt
Copy link
Member Author

I updated my version of fetch-configlet to this one you linked to and now it too fails like fetch-canonical_data_syncer - so there is at least consistency.

@ee7
Copy link
Member

ee7 commented Oct 11, 2020

I suspect that you have a bad GITHUB_TOKEN set. If I run a minimal example with a bad token:

#!/bin/bash

readonly LATEST='https://api.github.com/repos/exercism/canonical-data-syncer/releases/latest'

GITHUB_TOKEN="my_invalid_token"
HEADER="authorization: Bearer ${GITHUB_TOKEN}"

echo "Sending HEADER of '${HEADER}'"
curl --header "$HEADER" -s "$LATEST"

I see the output:

Sending HEADER of 'authorization: Bearer my_invalid_token'
{
  "message": "Bad credentials",
  "documentation_url": "https://docs.github.com/rest"
}

that is, I see the same response as you.

Questions:

  1. Are you seeing this issue just on your local machine, or during CI?
  2. Do you have GITHUB_TOKEN defined? That is, what is the output if you make the following diff to the repo's script and run it?
  if [ -z "${GITHUB_TOKEN}" ]
  then
      HEADER=''
+     echo "HEADER is the empty string"
  else
      HEADER="authorization: Bearer ${GITHUB_TOKEN}"
+     echo "HEADER contains token"
  fi

If you have a bad token defined then the behaviour you're seeing is the currently expected behaviour.

We could potentially add some code that falls back to an unauthenticated request if the authenticated one fails with

"message": "Bad credentials"

but that has the potential to use unauthenticated requests during CI, in which case we could start seeing mysterious CI failures again.

@verdammelt
Copy link
Member Author

Definitely appears to be User Error. I had a GITHUB_TOKEN set on this machine - not sure why or what it was for. I have corrected that (by, for now, setting it to a valid token) and fetch-configlet and fetch-canonical_data_syncer both work correctly.

Thank you for your time. I will close this issue.

@SleeplessByte
Copy link
Member

Maybe we can add to the script to run this (only in case of an error):

readonly LATEST='https://api.github.com/repos/exercism/canonical-data-syncer/releases/latest'

GITHUB_TOKEN="my_invalid_token"
HEADER="authorization: Bearer ${GITHUB_TOKEN}"

echo "Sending HEADER of '${HEADER}'"
curl --header "$HEADER" -s "$LATEST"

Might be an easy, nice to have, user improvement.

@ErikSchierboom
Copy link
Member

Wouldn't that expose the token in CI scenarios?

@ee7
Copy link
Member

ee7 commented Oct 18, 2020

Thank you for your time.

No problem. You're welcome.

Wouldn't that expose the token in CI scenarios?

Yes, it would.

But there's still probably some way to improve the ergonomics in this case. I'll open an issue to track this in the configlet repo, since the fetch-canonical_data_syncer script is based off fetch-configlet.

@ee7 ee7 changed the title fetch-canonical_data_syncer shell script fails to get URL of latest release fetch script: unclear error message with invalid GITHUB_TOKEN May 10, 2021
@ee7 ee7 added the status: duplicate This is tracked elsewhere label May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate This is tracked elsewhere
Projects
None yet
Development

No branches or pull requests

4 participants