Add a linter check for environment selectors.#2619
Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom Mar 10, 2026
Merged
Add a linter check for environment selectors.#2619openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
Conversation
openshift-ci
Bot
added
the
approved
bergmannf
force-pushed
the
add-linter-clusterselection
branch
2 times, most recently
from
b87e6c0 to
7abefbd
Compare
Contributor
|
/retest |
joshbranham
reviewed
Jan 5, 2026
bergmannf
force-pushed
the
add-linter-clusterselection
branch
from
37d0b45 to
cb0ad92
Compare
Contributor
|
Should we rebase and get this merged still @bergmannf ? |
This should prevent putting an invalid environment selector like 'stage' or 'int' into this repo. A workaround exists for syncsets for FedRAMP as those allow 'stage' until https://issues.redhat.com/browse/HCMSEC-2597 is implemented. Also updated to ubi9 & python3.12 to allow match expressions.
bergmannf
force-pushed
the
add-linter-clusterselection
branch
from
cb0ad92 to
7e45439
Compare
WalkthroughThe pull request upgrades the container image in the Makefile from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
@bergmannf: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate_template.py`:
- Around line 104-114: The helper expression_is_true improperly assumes
expression["values"] is a list of strings; normalize the incoming values first
(in expression_is_true) by coercing scalar strings or non-list inputs into a
list and then map each element to a lowercase string (e.g., values =
[str(v).lower() for v in <normalized list>]) before evaluating operators "In",
"NotIn", and "Eq" so selectors like values: "true" are handled correctly and the
FedRAMP selector logic adds the proper stage to valid_envs.
- Around line 179-203: The linter only validates
selectorSyncSet.matchExpressions but misses selectorSyncSet.matchLabels, so
extend the validation to also read sss.get("matchLabels", {}) and verify any
label with key environment_selector has a value present in valid_envs (after you
apply the fedramp_selector workaround via expression_is_true as currently done);
if the label value is not in valid_envs raise the same RuntimeError (use dirpath
and valid_environments in the message) and treat label values as strings
(matchLabels are scalar), referencing config["selectorSyncSet"], sss,
matchExpressions, matchLabels, environment_selector, fedramp_selector,
expression_is_true, valid_environments and add_sss_for() to locate the related
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80e14ad4-36e2-4407-8269-d7398fa1d467
📒 Files selected for processing (2)
Makefilescripts/generate_template.py
Comment on lines
+104
to
+114
| def expression_is_true(expression: dict[str, Any]) -> bool: | ||
| values = [v.lower() for v in expression["values"]] | ||
| match expression["operator"]: | ||
| case "In": | ||
| return "true" in values | ||
| case "NotIn": | ||
| return "false" in values | ||
| case "Eq": | ||
| return "true" in values | ||
| case _: | ||
| return False |
There was a problem hiding this comment.
Normalize values before evaluating FedRAMP selectors.
expression_is_true() assumes expression["values"] is already a list of strings, but Line 192 explicitly accepts scalar strings too. With values: "true", this helper lowercases characters instead of the selector value, so the FedRAMP workaround never adds stage to valid_envs and valid configs get rejected.
Suggested fix
def expression_is_true(expression: dict[str, Any]) -> bool:
- values = [v.lower() for v in expression["values"]]
+ raw_values = expression.get("values", [])
+ if isinstance(raw_values, list):
+ values = [str(v).lower() for v in raw_values]
+ else:
+ values = [str(raw_values).lower()]
match expression["operator"]:
case "In":
return "true" in values
case "NotIn":
return "false" in values
case "Eq":
return "true" in values
case _:
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def expression_is_true(expression: dict[str, Any]) -> bool: | |
| values = [v.lower() for v in expression["values"]] | |
| match expression["operator"]: | |
| case "In": | |
| return "true" in values | |
| case "NotIn": | |
| return "false" in values | |
| case "Eq": | |
| return "true" in values | |
| case _: | |
| return False | |
| def expression_is_true(expression: dict[str, Any]) -> bool: | |
| raw_values = expression.get("values", []) | |
| if isinstance(raw_values, list): | |
| values = [str(v).lower() for v in raw_values] | |
| else: | |
| values = [str(raw_values).lower()] | |
| match expression["operator"]: | |
| case "In": | |
| return "true" in values | |
| case "NotIn": | |
| return "false" in values | |
| case "Eq": | |
| return "true" in values | |
| case _: | |
| return False |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate_template.py` around lines 104 - 114, The helper
expression_is_true improperly assumes expression["values"] is a list of strings;
normalize the incoming values first (in expression_is_true) by coercing scalar
strings or non-list inputs into a list and then map each element to a lowercase
string (e.g., values = [str(v).lower() for v in <normalized list>]) before
evaluating operators "In", "NotIn", and "Eq" so selectors like values: "true"
are handled correctly and the FedRAMP selector logic adds the proper stage to
valid_envs.
Comment on lines
+179
to
+203
| # Verify that environment selectors make sense | ||
| sss = config["selectorSyncSet"] | ||
| valid_envs = valid_environments.copy() | ||
| expressions: list[dict[str, Any]] = sss.get("matchExpressions", []) if sss else [] | ||
| # FIXME: This is a workaround till https://issues.redhat.com/browse/HCMSEC-2597 is fixed. | ||
| # Until then 'stage' is a valid environment for fedramp | ||
| for expression in expressions: | ||
| if expression["key"] == fedramp_selector: | ||
| if expression_is_true(expression): | ||
| valid_envs.append('stage') | ||
| for expression in expressions: | ||
| if not expression["key"] == environment_selector: | ||
| continue | ||
| values: str|list[str] = expression["values"] | ||
| match values: | ||
| case list(x): | ||
| for value in x: | ||
| if value not in valid_envs: | ||
| raise RuntimeError(f"The environment value {value} for {dirpath} does not match a known environment: must be one of {valid_environments}") | ||
| case str(x): | ||
| if x not in valid_envs: | ||
| raise RuntimeError(f"The environment value {values} for {dirpath} does not match a known environment: must be one of {valid_environments}") | ||
| case _: | ||
| raise RuntimeError(f"Received invalid values {values} for {dirpath} for key: {environment_selector}") | ||
|
|
There was a problem hiding this comment.
matchLabels still bypasses the new environment lint.
The new check only walks selectorSyncSet.matchExpressions, but add_sss_for() also merges selectorSyncSet.matchLabels into the generated selector. A config that sets api.openshift.com/environment via matchLabels can still introduce an invalid value and pass this linter.
Suggested fix
sss = config["selectorSyncSet"]
valid_envs = valid_environments.copy()
+ match_labels: dict[str, Any] = sss.get("matchLabels", {}) if sss else {}
expressions: list[dict[str, Any]] = sss.get("matchExpressions", []) if sss else []
# FIXME: This is a workaround till https://issues.redhat.com/browse/HCMSEC-2597 is fixed.
# Until then 'stage' is a valid environment for fedramp
for expression in expressions:
if expression["key"] == fedramp_selector:
if expression_is_true(expression):
valid_envs.append('stage')
+
+ env_label = match_labels.get(environment_selector)
+ if env_label is not None and env_label not in valid_envs:
+ raise RuntimeError(
+ f"The environment value {env_label} for {dirpath} does not match a known environment: must be one of {valid_envs}"
+ )
+
for expression in expressions:
if not expression["key"] == environment_selector:
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Verify that environment selectors make sense | |
| sss = config["selectorSyncSet"] | |
| valid_envs = valid_environments.copy() | |
| expressions: list[dict[str, Any]] = sss.get("matchExpressions", []) if sss else [] | |
| # FIXME: This is a workaround till https://issues.redhat.com/browse/HCMSEC-2597 is fixed. | |
| # Until then 'stage' is a valid environment for fedramp | |
| for expression in expressions: | |
| if expression["key"] == fedramp_selector: | |
| if expression_is_true(expression): | |
| valid_envs.append('stage') | |
| for expression in expressions: | |
| if not expression["key"] == environment_selector: | |
| continue | |
| values: str|list[str] = expression["values"] | |
| match values: | |
| case list(x): | |
| for value in x: | |
| if value not in valid_envs: | |
| raise RuntimeError(f"The environment value {value} for {dirpath} does not match a known environment: must be one of {valid_environments}") | |
| case str(x): | |
| if x not in valid_envs: | |
| raise RuntimeError(f"The environment value {values} for {dirpath} does not match a known environment: must be one of {valid_environments}") | |
| case _: | |
| raise RuntimeError(f"Received invalid values {values} for {dirpath} for key: {environment_selector}") | |
| # Verify that environment selectors make sense | |
| sss = config["selectorSyncSet"] | |
| valid_envs = valid_environments.copy() | |
| match_labels: dict[str, Any] = sss.get("matchLabels", {}) if sss else {} | |
| expressions: list[dict[str, Any]] = sss.get("matchExpressions", []) if sss else [] | |
| # FIXME: This is a workaround till https://issues.redhat.com/browse/HCMSEC-2597 is fixed. | |
| # Until then 'stage' is a valid environment for fedramp | |
| for expression in expressions: | |
| if expression["key"] == fedramp_selector: | |
| if expression_is_true(expression): | |
| valid_envs.append('stage') | |
| env_label = match_labels.get(environment_selector) | |
| if env_label is not None and env_label not in valid_envs: | |
| raise RuntimeError( | |
| f"The environment value {env_label} for {dirpath} does not match a known environment: must be one of {valid_envs}" | |
| ) | |
| for expression in expressions: | |
| if not expression["key"] == environment_selector: | |
| continue | |
| values: str|list[str] = expression["values"] | |
| match values: | |
| case list(x): | |
| for value in x: | |
| if value not in valid_envs: | |
| raise RuntimeError(f"The environment value {value} for {dirpath} does not match a known environment: must be one of {valid_environments}") | |
| case str(x): | |
| if x not in valid_envs: | |
| raise RuntimeError(f"The environment value {values} for {dirpath} does not match a known environment: must be one of {valid_environments}") | |
| case _: | |
| raise RuntimeError(f"Received invalid values {values} for {dirpath} for key: {environment_selector}") |
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 197-197: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 200-200: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 202-202: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate_template.py` around lines 179 - 203, The linter only
validates selectorSyncSet.matchExpressions but misses
selectorSyncSet.matchLabels, so extend the validation to also read
sss.get("matchLabels", {}) and verify any label with key environment_selector
has a value present in valid_envs (after you apply the fedramp_selector
workaround via expression_is_true as currently done); if the label value is not
in valid_envs raise the same RuntimeError (use dirpath and valid_environments in
the message) and treat label values as strings (matchLabels are scalar),
referencing config["selectorSyncSet"], sss, matchExpressions, matchLabels,
environment_selector, fedramp_selector, expression_is_true, valid_environments
and add_sss_for() to locate the related logic.
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bergmannf, joshbranham The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should prevent putting an invalid environment selector like 'stg' or 'int' into this repo.
What type of PR is this?
feature
What this PR does / why we need it?
Additional checks when using environment selectors.
Which Jira/Github issue(s) this PR fixes?
Special notes for your reviewer:
Pre-checks (if applicable):
Tested latest changes against a cluster
Included documentation changes with PR
If this is a new object that is not intended for the FedRAMP environment (if unsure, please reach out to team FedRAMP), please exclude it with:
Summary by CodeRabbit