-
Notifications
You must be signed in to change notification settings - Fork 0
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
use github token when getting latest release #21
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.
Nice. Two thoughts:
- Do we want to make this optional?
- Do we want to make this an action input vs using secrets directly? Thinking about how other composite actions work I think this would be more standard?
Once this has merged to propagate I need to bump the rolling version (if this is non-breaking) or make a new rolling version (if this is breaking).
You mean something like a
Can you point me to an example where this is done? I'll also need to figure out how to actually make this work, and then it might also be better (safer) to just find the environment variable from the script rather than passing it as an argument. |
I mean if supplied use it and if it's null or not present don't use it. I'll have a look but limited as on a phone. Maybe codecov is an example? |
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.
This looks good I think will check out in more detail tonight.
I think this is non-breaking now and working except on windows where I don't have access to a good testing environment so am really fishing in the dark |
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.
Nice. Agree this looks good and is non breaking will update the rolling release tomorrow.
Windows has been a consistent issue here so not a surprise.
Description
If this works it would address the issue mentioned in epiforecasts/EpiNow2#769 (comment) - possibly causing the same issue elsewhere?
It tries to do so by sending a github token with the HTTP request to get the latest release version.
I have been able to test the unix version locally but not the windows one.
Checklist