Add a way to bypass the toolcache for kotlin and swift #1394
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
I don't think this requires a change note. |
This works by moving the logic to check for toolcache bypass out of creating the codeql instance. The logic now _may_ perform an API request in order to check what languages are in the repository. This check is redundant because the same call is being made later in the action when the actual list of languages is calculated.
aeisenberg
force-pushed
the
aeisenberg/bypass-toolcache-kotlin-swift
branch
from
476358e to
f79028a
Compare
Contributor
Author
|
I'm going to add a few more tests here since this logic is getting tricky. |
angelapwen
reviewed
Nov 24, 2022
Contributor
Author
|
I think I addressed all comments, added more tests, and fixed the bug I found. Hopefully, all tests will pass. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
adityasharad
left a comment
adityasharad
left a comment
There was a problem hiding this comment.
One more suggestion, otherwise looks reasonable. I will let Henry or Angela do a more detailed review however.
aeisenberg
force-pushed
the
aeisenberg/bypass-toolcache-kotlin-swift
branch
from
b36411b to
99d2473
Compare
aeisenberg
force-pushed
the
aeisenberg/bypass-toolcache-kotlin-swift
branch
from
99d2473 to
ad7ca9b
Compare
henrymercer
reviewed
Nov 24, 2022
Contributor
henrymercer
left a comment
henrymercer
left a comment
There was a problem hiding this comment.
This generally looks great.
Some questions:
- Should we revert this change once the next release is fully rolled out and we no longer need to bypass the toolcache for Kotlin / Swift? It does look like there's some complexity here that we wouldn't want to keep around.
- Do we want to add a PR check that runs some integration tests, or are we happy with the unit tests here? Personally I'd be ok with unit tests here plus some manual testing.
If a user explicitly includes java in their language inputs, always make an api call to check for kotlin in the repo. Also, add some suggestions from code reviews.
aeisenberg
force-pushed
the
aeisenberg/bypass-toolcache-kotlin-swift
branch
from
fede016 to
102e01d
Compare
henrymercer
approved these changes
Nov 25, 2022
This was referenced Nov 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This works by moving the logic to check for toolcache bypass out of
creating the codeql instance. The logic now may perform an API request
in order to check what languages are in the repository. This check is
redundant because the same call is being made later in the action when
the actual list of languages is calculated.
Commit-by-commit review is suggested.
Even though it is inefficient to add this extra API call, I am leaning towards leaving it in because refactoring to perform only one API call would make the code messier, I ensure that the extra API call is made only if necessary, and we expect that this workaround will be temporary.
Merge / deployment checklist