-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add hello world workflow to help diagnose GitHub token issues #86
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,37 @@ | |||
from consts import GITHUB_REPO, GITHUB_TOKEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this go in scripts/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, it should go in scripts/
. The awkward thing about scripts/
is that there's no easy way to import these constants (or am I missing something?). On the other hand, I don't really need the constants (I can use os.getenv(...)
instead), I can use print
instead of setup_logging
, and I don't need get_github_branch_name
. Let me try moving it to scripts and see if that's not too bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new commit moves it to scripts/
- @msaroufim, please have another look when you have a moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine to merge if this is helpful although this is suggesting that the errors here https://github.com/gpu-mode/discord-cluster-manager/blob/main/src/discord-cluster-manager/consts.py#L7-L14 are not informative enough
@b9r5 I'm a bit confused, what does "GitHub API error: 404 - Not Found" imply here? Otherwise I do like this. We should probably update the README as well to include this. |
@alexzhang13, that's a message that appears because GitHub can't find the hello world workflow. I can make the error messages more informative. |
So this PR started as an attempt to write a script that would diagnose problems with a developer's dev setup. That was at a time when I thought that Andre was having trouble with Discord tokens. I'm wondering if I should enhance the script to sanity check the developer's environment variables, including Discord tokens, GitHub tokens, DB credentials, etc. But, I don't really know if that sort of script is worth it at this time. It might be better just to help people out on the staging server. @alexzhang13 @AndreSlavescu @msaroufim, what do you think? Should I keep going with this PR? Should I improve the error messages and merge it (which would be a smaller amount of effort)? Should I just wait until after Jan 1 and then we'll figure out what needs to be worked on? |
@b9r5 If you have the time, by all means this would be super helpful for anyone. It would also reduce the amount of questions related to setup for sure. I also don't think this is conditional on any post Jan 1st changes so it should be fine to get in now if it isn't too much work. Also the GitHub API Error is fine. It probably isn't going to pop in the future, so I don't mind not explicitly handling it. |
Description
This PR adds a hello world workflow in GitHub, and a script to run the workflow. I am hoping that this will help diagnose problems that new contributors have with getting set up.
Unfortunately, I don't think it's possible to test the script until this PR is merged, because the workflow isn't showing up for me when I run the script:
Checklist
Before submitting this PR, ensure the following steps have been completed:
/verifyruns
on your own server./verifyruns
.runs may take a little longer. The Modal run is typically quick.)
For more information on running a cluster bot on your own server, see
README.md.