FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Re-enable new style of tracing by edoardopirovano · Pull Request #770 · github/codeql-action · GitHub

Re-enable new style of tracing - #770

Merged
edoardopirovano merged 1 commit into
mainfrom
enable-new-tracing
Nov 4, 2021
Merged

edoardopirovano merged 1 commit into
mainfrom
enable-new-tracing

Conversation

edoardopirovano commented Oct 12, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

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

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

edoardopirovano requested a review from a team as a code owner October 12, 2021 12:49

Copy link
Copy Markdown
Contributor

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 ...

alexet commented Oct 12, 2021

Copy link
Copy Markdown

I would like the switch to be made separately to a CLI release so any issues are attributed to the correct cause more easily.

edoardopirovano commented Oct 12, 2021
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/codeql.ts Outdated

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Per the discussion above, I will hold off on merging until after GitHub Universe.

jbj commented Oct 26, 2021

Copy link
Copy Markdown

Cc @criemen.

criemen commented Oct 27, 2021

Copy link
Copy Markdown
Contributor

Do we now have an integration test for this that tests that unsetting the entire environment doesn't break extraction of C++ or C#?

criemen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Leaving a blocking review until the Go special casing discussed on slack has been resolved.

Copy link
Copy Markdown
Contributor Author

Do we now have an integration test for this that tests that unsetting the entire environment doesn't break extraction of C++ or C#?

This is a good point. There's a test in semmle-code on the indirect build tracing feature that the Action is using, but it's probably worth having an end-to-end integration test with the Action here. I have added this unsetting to one of our existing integration tests.

Leaving a blocking review until the Go special casing discussed on slack has been resolved.

Thanks. I believe this has now been resolved with no changes needed as we have decided we will not special case Go going forward.

criemen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One minor nitpick then, LGTM otherwise

Copy link
Copy Markdown
Contributor Author

Aargh, looks like something is still broken 😠

Let me investigate what is going on...

Copy link
Copy Markdown
Contributor Author

Aargh, looks like something is still broken 😠

Let me investigate what is going on...

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 codeql-action release tomorrow per @alexet's suggestion so we can pinpoint any issues that may arise to this PR.

edoardopirovano merged commit ae28432 into main Nov 4, 2021
edoardopirovano deleted the enable-new-tracing branch November 4, 2021 10:03
github-actions Bot mentioned this pull request Nov 4, 2021
5 tasks
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
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.

7 participants


Back | FazBrowse Home | New Git URL