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

Correctly report CodeQL version when using cache #1259

Merged
merged 4 commits into from
Oct 5, 2022

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented Sep 19, 2022

Merge / deployment checklist

A one-line change to correctly update the version reported to our telemetry when we used a cached CodeQL bundle. Also updates the test suite to check the return value of the setupCodeQL function, which is what feeds into the version reported to our telemetry.

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

@angelapwen angelapwen requested a review from a team as a code owner September 19, 2022 19:26
@adityasharad
Copy link
Contributor

I am unsure how to test this without waiting for the next CodeQL release and monitoring our telemetry during the period of time when the older cached version is used.

There are some unit tests that call setupCodeQL, but use it to configure CodeQL without checking the return value. See if you can extend those, or write simpler new ones, so that they check the return value from this function, while mocking different conditions of the toolcache/GitHub API/user config.

I'm also not sure if this warrants a changelog post because the only visible changes are to internal users viewing our telemetry.

Nothing user-facing, so indeed no change note.

@adityasharad
Copy link
Contributor

I am unsure how to test this without waiting for the next CodeQL release and monitoring our telemetry during the period of time when the older cached version is used.

There are some unit tests that call setupCodeQL, but use it to configure CodeQL without checking the return value. See if you can extend those, or write simpler new ones, so that they check the toolsVersion in the return value from this function, while mocking different conditions of the toolcache/GitHub API/user config.

I'm also not sure if this warrants a changelog post because the only visible changes are to internal users viewing our telemetry.

Nothing user-facing, so indeed no change note.

@adityasharad
Copy link
Contributor

I am unsure how to test this without waiting for the next CodeQL release and monitoring our telemetry during the period of time when the older cached version is used.

I see some unit tests that call setupCodeQL, but use it to configure CodeQL without checking the return value. See if you can extend those, or write simpler new ones, so that they check the toolsVersion in the return value from this function, while mocking different conditions of the toolcache/GitHub API/user config. By mocking these conditions and checking the toolsVersion, you'll be able to simulate what gets reported to the telemetry and make sure we are handling the various possible cases, without having to actually reach that situation during a real workflow run.

I'm also not sure if this warrants a changelog post because the only visible changes are to internal users viewing our telemetry.

Nothing user-facing, so indeed no change note.

julioAk47
julioAk47 previously approved these changes Sep 23, 2022
@angelapwen angelapwen force-pushed the angelapwen/version-report-cache branch from c19780f to cc2606e Compare October 4, 2022 22:21
@aeisenberg
Copy link
Contributor

You could test this by running the the action in a testing repo using this branch. But, the changes look reasonable to me. I suggest we merge and then check if it is working.

@angelapwen angelapwen merged commit 24c8de1 into main Oct 5, 2022
@angelapwen angelapwen deleted the angelapwen/version-report-cache branch October 5, 2022 16:16
@github-actions github-actions bot mentioned this pull request Oct 6, 2022
7 tasks
angelapwen added a commit that referenced this pull request Oct 6, 2022
* Update changelog and version after v2.1.26

* Update checked-in dependencies

* Don't check for Go logs on failure (#1279)

* Update supported GitHub Enterprise Server versions. (#1275)

Co-authored-by: GitHub <noreply@github.com>

* TRAP Caching: Add timeouts to upload/download operations

* Add logging statements declaring state of the cli_config_file_enabled

It's possible to determine this otherwise, but this makes it easier to
spot.

* Avoid using single value as array

The user config parser in the CLI doesn't yet support it.

* Extract logging statements to separate function

* Correctly report CodeQL version when using cache (#1259)

* Correctly report CodeQL version when using cache

* Add JS generated files

* Add test for return value of `setupCodeQL`

* Fill in missing return value comment

* Convert "Invalid source root" errors to UserErrors

* Add changelog note for Go extraction reconciliation (#1286)

* Add changelog note for Go extraction reconciliation

* Update CHANGELOG.md

Co-authored-by: Henry Mercer <henrymercer@github.com>

* Update CHANGELOG.md

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

* Tweaks from PR review

Co-authored-by: Henry Mercer <henrymercer@github.com>
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

* Update changelog for v2.1.27

Co-authored-by: github-actions[bot] <github-actions@github.com>
Co-authored-by: Chuan-kai Lin <cklin@github.com>
Co-authored-by: Angela P Wen <angelapwen@github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: GitHub <noreply@github.com>
Co-authored-by: Edoardo Pirovano <edoardo.pirovano@gmail.com>
Co-authored-by: Edoardo Pirovano <6748066+edoardopirovano@users.noreply.github.com>
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
Co-authored-by: Henry Mercer <henrymercer@github.com>
angelapwen added a commit that referenced this pull request Oct 7, 2022
* Merge main into releases/v2 (#1287)

* Update changelog and version after v2.1.26

* Update checked-in dependencies

* Don't check for Go logs on failure (#1279)

* Update supported GitHub Enterprise Server versions. (#1275)

Co-authored-by: GitHub <noreply@github.com>

* TRAP Caching: Add timeouts to upload/download operations

* Add logging statements declaring state of the cli_config_file_enabled

It's possible to determine this otherwise, but this makes it easier to
spot.

* Avoid using single value as array

The user config parser in the CLI doesn't yet support it.

* Extract logging statements to separate function

* Correctly report CodeQL version when using cache (#1259)

* Correctly report CodeQL version when using cache

* Add JS generated files

* Add test for return value of `setupCodeQL`

* Fill in missing return value comment

* Convert "Invalid source root" errors to UserErrors

* Add changelog note for Go extraction reconciliation (#1286)

* Add changelog note for Go extraction reconciliation

* Update CHANGELOG.md

Co-authored-by: Henry Mercer <henrymercer@github.com>

* Update CHANGELOG.md

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

* Tweaks from PR review

Co-authored-by: Henry Mercer <henrymercer@github.com>
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

* Update changelog for v2.1.27

Co-authored-by: github-actions[bot] <github-actions@github.com>
Co-authored-by: Chuan-kai Lin <cklin@github.com>
Co-authored-by: Angela P Wen <angelapwen@github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: GitHub <noreply@github.com>
Co-authored-by: Edoardo Pirovano <edoardo.pirovano@gmail.com>
Co-authored-by: Edoardo Pirovano <6748066+edoardopirovano@users.noreply.github.com>
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
Co-authored-by: Henry Mercer <henrymercer@github.com>

* Update changelog and version after v2.1.27

* Update checked-in dependencies

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions@github.com>
Co-authored-by: Chuan-kai Lin <cklin@github.com>
Co-authored-by: Angela P Wen <angelapwen@github.com>
Co-authored-by: GitHub <noreply@github.com>
Co-authored-by: Edoardo Pirovano <edoardo.pirovano@gmail.com>
Co-authored-by: Edoardo Pirovano <6748066+edoardopirovano@users.noreply.github.com>
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
Co-authored-by: Henry Mercer <henrymercer@github.com>
@github-actions github-actions bot mentioned this pull request Oct 14, 2022
7 tasks
@aeisenberg aeisenberg mentioned this pull request Oct 14, 2022
3 tasks
@github-actions github-actions bot mentioned this pull request Oct 17, 2022
8 tasks
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.

4 participants