-
Notifications
You must be signed in to change notification settings - Fork 309
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
Re-enable new style of tracing #770
Conversation
Hmm, as much as I'd like getting rid of the old code, perhaps we shouldn't suddenly enable the new way to do things in production the moment the new CLI release timed to coincide with GitHub Universe becomes available ... |
I would like the switch to be made separately to a CLI release so any issues are attributed to the correct cause more easily. |
Both sensible points. How about we merge this after the next CLI release and do an Action release shortly after (and after GitHub Universe)? That way we'll have two versions of the Action using the same CLI but with and without the new style of tracing. |
798e641
to
25619e7
Compare
Thanks for the review! Per the discussion above, I will hold off on merging until after GitHub Universe. |
25619e7
to
7f0e915
Compare
Cc @criemen. |
Do we now have an integration test for this that tests that unsetting the entire environment doesn't break extraction of C++ or C#? |
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.
Leaving a blocking review until the Go special casing discussed on slack has been resolved.
This is a good point. There's a test in
Thanks. I believe this has now been resolved with no changes needed as we have decided we will not special case Go going forward. |
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.
One minor nitpick then, LGTM otherwise
Aargh, looks like something is still broken 😠 Let me investigate what is going on... |
e0fbdc2
to
2c3b659
Compare
The root cause of this was https://github.com/github/codeql-coreql-team/issues/1719. I don't think this needs to block releasing this as things were already broken there. If there's no objections, I'll merge this and do a separate |
2c3b659
to
2fd825b
Compare
2fd825b
to
5156a89
Compare
In #766, I temporarily disabled using the new style of tracing introduced by #744 as some issues were identified with it.
These issues have now been fixed by myself and @alexet in the internal PRs https://github.com/github/semmle-code/pull/40519 and https://github.com/github/semmle-code/pull/40515, both of which will be included in the upcoming 2.7.0 release of the CodeQL CLI.
Therefore, it should be safe for the
codeql-action
to use indirect and multi-language tracing starting with that release. This PR sets the version flag accordingly.cc. @hmakholm and @jbj as they reviewed the internal PR.
Merge / deployment checklist