Skip to content

Actions: Make Env non-abstract #19675

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

Merged
merged 1 commit into from
Jun 5, 2025
Merged

Actions: Make Env non-abstract #19675

merged 1 commit into from
Jun 5, 2025

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Jun 5, 2025

class Env was previously abstract with no concrete descendants, so user queries like any(Env e | ...) would never produce results.

In the JS library the corresponding class derived from YamlNode and has concrete descendants representing workflow-, job- and step-level env nodes. However these are dubiously useful since you can always just use any(Step s).getEnv() to achieve the same result. Since EnvImpl already fully characterises an env node, I simply make the class concrete.

`class Env` was previously abstract with no concrete descendants, so user queries like `any(Env e | ...)` would never produce results.

In the JS library the corresponding class derived from `YamlNode` and has concrete descendants representing workflow-, job- and step-level `env` nodes. However these are dubiously useful since you can always just use `any(Step s).getEnv()` to achieve the same result. Since `EnvImpl` already fully characterises an `env` node, I simply make the class concrete.
@Copilot Copilot AI review requested due to automatic review settings June 5, 2025 09:21
@smowton smowton requested a review from a team as a code owner June 5, 2025 09:21
@github-actions github-actions bot added the Actions Analysis of GitHub Actions label Jun 5, 2025
Copy link
Contributor

@Copilot 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 makes the Env class instantiable (no longer abstract) so that queries like any(Env e | ...) can return results, and updates its documentation comment.

  • Removed the abstract keyword from Env to allow concrete instances.
  • Refined the class doc comment for Env.
Comments suppressed due to low confidence (2)

actions/ql/lib/codeql/actions/Ast.qll:53

  • [nitpick] Consider clarifying the doc comment to follow project conventions, e.g., /** Represents an env block at the workflow, job, or step level. */ for consistency and clarity.
/** An `env` in workflow, job or step. */

actions/ql/lib/codeql/actions/Ast.qll:54

  • Add a CodeQL test to verify that Env is now concrete and queryable (e.g., ensure any(Env e) returns expected nodes) to prevent regressions.
class Env extends AstNode instanceof EnvImpl {

@smowton smowton added the no-change-note-required This PR does not need a change note label Jun 5, 2025
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Makes sense.

@smowton smowton merged commit fbae306 into main Jun 5, 2025
14 of 15 checks passed
@smowton smowton deleted the smowton/fix/abstract-env branch June 5, 2025 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Actions Analysis of GitHub Actions no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants