exasol / github-keeper

Tool for unifying our GitHub repository setup. Superseded by github-issue-adapter
MIT License
2 stars 1 forks source link

github-keeper fails with stacktrace for no obvious reason #59

Closed Nicoretti closed 1 year ago

Nicoretti commented 2 years ago

Details

github-keeper crashes with no obvious reason.

screenshot

Steps to reproduce

  1. Setup a projekt-keeper credentials file with outdated/invalid github credentials
  2. Execute github-keeper

Expected behavior

github-keeper is reporting that the credentials are invalid and should be updated.

Actual behavior

github-keeper crashes with a stacktrace

Root Cause

root cause seems to be the usage of expired github credentials (e.g. gh token).

Note

:warning: Apparently github-keeper is retrieving the credentials from the project-keeper credentials file, this is far from being obvious.

Tools strategy
release-droid file ~/.release-droid/credentials
github-keeper file ~/.release-droid/credentials
product-integration-tool-chest / python file ~/.release-droid/credentials
product-integration-tool-chest / shell scripts uses tool gh, which evaluates environment variable GH_TOKEN
project-overview environment variable GITHUB_TOKEN
project-keeper (no access to github, yet)
Build scripts, generated by project-keeper environment variable GITHUB_TOKEN
Nicoretti commented 2 years ago

Consider adding keyring for storing the secret(s).

ckunki commented 1 year ago

Hello @Nicoretti, cmd/githubClientProvider.go evaluates ~/.release-droid/credentials.

keyring could raise the question how to support java applications such as release droid. How should we proceed here?

Nicoretti commented 1 year ago

@ckunki a good error message if the release-droid/credentails file is missing would be the bare minimum. It is far from obvious that github-keeper needs the credential file of another tool.

ckunki commented 1 year ago

Hi Nico, I definitely agree to a good error message as bare minimum. I must admit that I still feel to have too little understanding about how closely different tools are linked if such a linkage is desired or should be avoided.

Nicoretti commented 1 year ago

@ckunki from what I understand, there is no required linkage between those two tools. From what I can tell it just "reuses" the credentials file for convince (de duplication) reasons. Still having a more general file name/location e.g. exasol/tools/credentials or the option to specify the credentials file in the config file or as command line argument would be a cleaner solution in my opinion.

ckunki commented 1 year ago

I found more linked tools, see main description of the ticket. On the first glance, evaluating file ~/.release-droid/credentials seems to be used by the majority. Still I very much agree on first improving the error message. And also the file path could be more generic, yes. Maybe we could use a more sophisticated strategy?

  1. if file ~/.release-droid/credentials is available and contains github_oauth_access_token, then use it
  2. check environment variable GITHUB_TOKEN
  3. check environment variable GH_TOKEN

In all cases: report precise error on failure.

BTW: I used a different approach. With the following bash function you can set an environment variable from file ~/.release-droid/credentials without requiring your preferred tool to support or even know of the file.

function github-token-from-release-droid-credentials () {
    export GH_TOKEN=$(cat ~/.release-droid/credentials \
              | grep github_oauth_access_token \
              | cut -d= -f2)
}

Maybe this would be an option to avoid a tight coupling between our zoo of tools.

Nicoretti commented 1 year ago

A more sophisticated strategy definitely would help. Still the major issue in my opinion is that the configuration file of another tool is used. Having a general config file for common settings of our tooling e.g. .config/exasol/tools/config.ini would be the cleanest solution besides using the keyring. As a migration strategy we could support both, old and new config file location and issue a warning if only the old one contains a config. This would enable us to migrate the dependent tools individually.