Skip to content

Conversation

@kaspersv
Copy link
Contributor

This PR contains a few small code improvements related to overlay analysis from review comments on #3332 and #3333. The PR is a pure refactoring and should preserve all existing behaviours.

Risk assessment

For internal use only. Please select the risk level of this change:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Workflow types:

  • Advanced setup - Impacts users who have custom CodeQL workflows.
  • Managed - Impacts users with dynamic workflows (Default Setup, CCR, ...).

Products:

  • Code Scanning - The changes impact analyses when analysis-kinds: code-scanning.
  • Code Quality - The changes impact analyses when analysis-kinds: code-quality.

Environments:

  • Dotcom - Impacts CodeQL workflows on github.com.
  • GHES - Impacts CodeQL workflows on GitHub Enterprise Server.

How did/will you validate this change?

  • Test repository - This change will be tested on a test repository before merging.
  • Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

  • Feature flags - All new or changed code paths can be fully disabled with corresponding feature flags.
  • Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.

How will you know if something goes wrong after this change is released?

  • Telemetry - I rely on existing telemetry or have made changes to the telemetry.
    • Dashboards - I will watch relevant dashboards for issues after the release. Consider whether this requires this change to be released at a particular time rather than as part of a regular release.
    • Alerts - New or existing monitors will trip if something goes wrong with this change.

Are there any special considerations for merging or releasing this change?

  • No special considerations - This change can be merged at any time.
  • Special considerations - This change should only be merged once certain preconditions are met. Please provide details of those or link to this PR from an internal issue.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

@github-actions github-actions bot added the size/S Should be easy to review label Nov 27, 2025
@kaspersv kaspersv force-pushed the kaspersv/overlay-minor-comments branch from e00fc78 to 766f142 Compare November 27, 2025 14:55
@kaspersv kaspersv force-pushed the kaspersv/overlay-minor-comments branch from 766f142 to 58c5954 Compare November 27, 2025 14:56
@kaspersv kaspersv marked this pull request as ready for review November 28, 2025 07:50
@kaspersv kaspersv requested a review from a team as a code owner November 28, 2025 07:50
Copilot AI review requested due to automatic review settings November 28, 2025 07:50
Copilot finished reviewing on behalf of kaspersv November 28, 2025 07:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR contains small code improvements for overlay analysis functionality, addressing review feedback from previous PRs. The changes are pure refactoring with no behavioral modifications.

  • Renamed getMemoryFlagValue to getCodeQLMemoryLimit to better reflect its purpose
  • Extracted magic number 5 * 1024 into a named constant OVERLAY_MINIMUM_MEMORY_MB with documentation
  • Fixed alphabetical ordering of feature flags in the enum and config object

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/util.ts Renamed function getMemoryFlagValue to getCodeQLMemoryLimit and updated JSDoc to clarify it returns the memory limit CodeQL is allowed to use
src/init-action.ts Updated function call to use renamed getCodeQLMemoryLimit
src/feature-flags.ts Reordered OverlayAnalysisSkipResourceChecks and OverlayAnalysisSwift entries to maintain alphabetical order in both enum and config
src/config-utils.ts Extracted magic number into OVERLAY_MINIMUM_MEMORY_MB constant with documentation, and updated function call to use renamed getCodeQLMemoryLimit
src/config-utils.test.ts Updated test stub to use renamed getCodeQLMemoryLimit
lib/*.js Auto-generated JavaScript files mirroring the TypeScript source changes

// src/config-utils.ts
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB = 2e4;
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_BYTES = OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB * 1e6;
var OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Unused variable OVERLAY_MINIMUM_MEMORY_MB.

Copilot uses AI. Check for mistakes.
// src/config-utils.ts
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB = 2e4;
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_BYTES = OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB * 1e6;
var OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Unused variable OVERLAY_MINIMUM_MEMORY_MB.

Copilot uses AI. Check for mistakes.
// src/config-utils.ts
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB = 2e4;
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_BYTES = OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB * 1e6;
var OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Unused variable OVERLAY_MINIMUM_MEMORY_MB.

Copilot uses AI. Check for mistakes.
// src/config-utils.ts
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB = 2e4;
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_BYTES = OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB * 1e6;
var OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Unused variable OVERLAY_MINIMUM_MEMORY_MB.

Copilot uses AI. Check for mistakes.
// src/config-utils.ts
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB = 2e4;
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_BYTES = OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB * 1e6;
var OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Unused variable OVERLAY_MINIMUM_MEMORY_MB.

Copilot uses AI. Check for mistakes.
// src/config-utils.ts
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB = 2e4;
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_BYTES = OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB * 1e6;
var OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Unused variable OVERLAY_MINIMUM_MEMORY_MB.

Copilot uses AI. Check for mistakes.
// src/config-utils.ts
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB = 2e4;
var OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_BYTES = OVERLAY_MINIMUM_AVAILABLE_DISK_SPACE_MB * 1e6;
var OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Unused variable OVERLAY_MINIMUM_MEMORY_MB.

Copilot uses AI. Check for mistakes.
Base automatically changed from kaspersv/overlay-no-resource-checks-option to main November 28, 2025 09:01
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

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

Thanks for addressing these small points. One suggestion, but not blocking.

Comment on lines +63 to +70
/**
* The minimum memory (in MB) that must be available for CodeQL to perform overlay
* analysis. If CodeQL will be given less memory than this threshold, then the
* action will not perform overlay analysis unless overlay analysis has been
* explicitly enabled via environment variable.
*/
const OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;

Copy link
Member

Choose a reason for hiding this comment

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

Minor:

Suggested change
/**
* The minimum memory (in MB) that must be available for CodeQL to perform overlay
* analysis. If CodeQL will be given less memory than this threshold, then the
* action will not perform overlay analysis unless overlay analysis has been
* explicitly enabled via environment variable.
*/
const OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;
/**
* The minimum memory (in MB) that must be available for CodeQL to perform overlay
* analysis. If CodeQL will be given less memory than this threshold, then the
* action will not enable overlay analysis unless overlay analysis has been
* explicitly requested via environment variable.
*/
const OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;

More generally, I'd maybe avoid this level of detail because it is difficult to maintain and keep synchronised with code changes elsewhere. Perhaps something like:

Suggested change
/**
* The minimum memory (in MB) that must be available for CodeQL to perform overlay
* analysis. If CodeQL will be given less memory than this threshold, then the
* action will not perform overlay analysis unless overlay analysis has been
* explicitly enabled via environment variable.
*/
const OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;
/**
* The minimum memory (in MB) that must be available for us to automatically
* enable overlay analysis.
*/
const OVERLAY_MINIMUM_MEMORY_MB = 5 * 1024;

@kaspersv kaspersv merged commit 23da732 into main Nov 28, 2025
241 checks passed
@kaspersv kaspersv deleted the kaspersv/overlay-minor-comments branch November 28, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Should be easy to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants