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

--new-pr does not check for valid GitHub user name #3642

Closed
ocaisa opened this issue Apr 9, 2021 · 4 comments · Fixed by #3644
Closed

--new-pr does not check for valid GitHub user name #3642

ocaisa opened this issue Apr 9, 2021 · 4 comments · Fixed by #3644
Milestone

Comments

@ocaisa
Copy link
Member

ocaisa commented Apr 9, 2021

I was just opening an easyconfig PR and noticed that --new-pr doesn't seem to check that there is a valid GitHub user name set (any more?..EB v4.3.4) and tries to use None

alanc@~$ eb --new-pr line-prof.eb
== Temporary log file in case of crash /tmp/eb-za20dzv9/easybuild-vg4d9x65.log
== fetching branch 'develop' from https://github.com/easybuilders/easybuild-easyconfigs.git...
== copying files to /tmp/eb-za20dzv9/git-working-dirdxszylm8/easybuild-easyconfigs...
== pushing branch '20210409123147_new_pr_line_profiler310' to remote 'github_None_VeRJa' ([email protected]:None/easybuild-easyconfigs.git)
ERROR: Failed to push branch '20210409123147_new_pr_line_profiler310' to GitHub ([email protected]:None/easybuild-easyconfigs.git): Cmd('git') failed due to: exit code(128)
  cmdline: git push --porcelain github_None_VeRJa 20210409123147_new_pr_line_profiler310
  stderr: 'fatal: Could not read from remote repository.
@ocaisa
Copy link
Member Author

ocaisa commented Apr 9, 2021

Also true for --check-github, it tries to do GitHub operations with no valid user name

@ocaisa
Copy link
Member Author

ocaisa commented Apr 9, 2021

I think we can probably add a check in https://github.com/easybuilders/easybuild-framework/blob/develop/easybuild/tools/github.py#L953 that the target_account is not None, and don't attempt the push in this case (and say why).
Similar for gists in https://github.com/easybuilders/easybuild-framework/blob/develop/easybuild/tools/github.py#L559?

@boegel boegel added this to the next release (4.3.5?) milestone Apr 9, 2021
@boegel
Copy link
Member

boegel commented Apr 9, 2021

Hmm, can you check if this a regression we introduced at some point?

We can definitely do better, but there are some caveats too: if --new-pr --dry-run is used, we should still do all the local stuff, and in that case we don't actually need a GitHub token (or a GitHub user name). I guess in that case we can/should still try to produce a warning.

I'm a bit surprised by this though: when --new-pr is run, EB will try and grab the GitHub token, and it needs the GitHub username for that...

@ocaisa
Copy link
Member Author

ocaisa commented Apr 9, 2021

So I use AddKeysToAgent Confirm in my ssh config and I can see that when using --check-github, there are 2 usages of the key for push access and one usage for gists when a github user is given. With no user given, it still tries to use the key once when checking for push access so there is something not quite right there. This behaviour is there back to 4.0.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants