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

Print that we are downloading codeql and it may take a while #169

Merged
merged 2 commits into from Sep 7, 2020

Conversation

robertbrignull
Copy link
Contributor

The CodeQL download can take quite a while on a normal internet connection and GitHub appears to also deliberately throttle the download speed. This means it can easily take 10+ minutes, so we need to output something nice. Before this PR the last thing that gets printed is "checking cache" and "not found" which is not informative enough.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@aeisenberg
Copy link
Contributor

10 minutes to download??? I almost wonder if it's better to build the bundle ourselves.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you have looks perfectly reasonable.

I notice that there is a comment on toolcacheDownloadTool to remove once actions/toolkit#530 is merged and released. Looks like it has already, so perhaps this is a good time to change? And perhaps going through the tool-cache package would avoid throttling?

@robertbrignull
Copy link
Contributor Author

Looks like it has already

actions/toolkit#530 doesn't look merged to me. I think the confusion is that it mentions #123 which is merged so there's a big "merged" icon, but it's for the other PR.

And perhaps going through the tool-cache package would avoid throttling?

The code in this file was copied from the toolcache, and then slightly modified with the changes from that PR, so changing to use the tool-cache properly wouldn't change anything. The lack of download speed is downloading the release artifact from github.com and I'm pretty sure is imposed at their end and not caused by the method in which we are doing the downloading.

@aeisenberg
Copy link
Contributor

My mistake...you're right. It's not merged.

@robertbrignull
Copy link
Contributor Author

@sampart can you review? Or @aeisenberg please feel free to also review. Currently I think putting this message in is better than nothing and I think we should get this in as a priority.

Sometimes the download can take so long that you'll think it's broken, so outputting progress updates on some kind of regular schedule would be better. But I think we can investigate that separately.

@robertbrignull robertbrignull merged commit 8c43427 into main Sep 7, 2020
@robertbrignull robertbrignull deleted the codeql_download_info branch September 7, 2020 13:32
@github-actions github-actions bot mentioned this pull request Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants