-
Notifications
You must be signed in to change notification settings - Fork 22
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 support for creating PR's in ghes and behind proxy #40
Conversation
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.
Thanks for the contribution!
Left some comments inline.
options.baseUrl = `${baseurl}/api/v3`; | ||
} | ||
|
||
const proxy = process.env.https_proxy || process.env.HTTPS_PROXY; |
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.
Is this some kind of standardized env variable? I think I'd prefer this to be a input because it is easier to test.
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.
yes it is standardized env variable. i have seen various other github actions doing very similar.
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.
Okay, can we document this in the README then please?
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.
sure will do.
Hmm, I never tried running these CI scripts from a fork, only from my own branches. What is your appetite for trying fix this? I think it should be doable by adding the fork that exists under your username and explicitly pushing to that so we can open a pull-request. It is fine if you don't want to but I also can't promise that I'll find time very soon to try and play around with it. |
can you approve the above PR and see if that fixes it? Not sure what you mean by - "I think it should be doable by adding the fork that exists under your username and explicitly pushing to that so we can open a pull-request."? i am actually pushing to my fork only. |
Sorry, I should have been more clear. You are the first one to contribute which shows that my CI scripts don't work with forks 😅 What I meant with "I think it should be doable by adding the fork that exists under your username and explicitly pushing to that so we can open a pull-request" is that the CI scripts could be modified to try and push to a remote that is under the username of whoever triggered the workflow. There is a However, maybe this kind of integration testing is also overkill though and should just be removed :) |
Unfortunately, I don't currently have time for this. i just want this PR to be merged so that we can use this github action. |
I will take your commits and push them to a branch on the repo. That should be good enough for now. Thanks for the contribution! |
Closed in favor of #41. |
This PR addresses the issue #39