chore(release): fix typo that results in is_version_pr being falsely evaluated (#2468) - #2470
Merged
hopehadfield merged 1 commit intoMar 5, 2026
Conversation
…evaluated (redhat-developer#2469) Signed-off-by: Hope Hadfield <hhadfiel@redhat.com> Co-authored-by: Hope Hadfield <hhadfiel@redhat.com>
hopehadfield
requested review from
a team,
batzionb,
gciavarrini and
mareklibra
as code owners
March 5, 2026 17:02
Review Summary by QodoAdd auth token and custom filters to Loki workflow logs provider
WalkthroughsDescription• Add authentication token support to Loki queries with Bearer token header • Implement custom SSL certificate handling via rejectUnauthorized config option • Add configurable log pipeline filters for flexible Loki query filtering • Change default log stream selector from service_name to openshift_log_type • Fix GitHub Actions workflow variable reference bug in version PR detection Diagramflowchart LR
A["LokiProvider Config"] -->|"token, rejectUnauthorized, logPipelineFilters"| B["Enhanced LokiProvider"]
B -->|"Bearer token header"| C["Authenticated Loki API Request"]
B -->|"Custom Agent with SSL config"| C
B -->|"Pipeline filters appended"| D["Flexible Log Queries"]
E["GitHub Actions Workflow"] -->|"Fix variable reference"| F["Correct Version PR Detection"]
File Changes1. workspaces/orchestrator/plugins/orchestrator-backend-module-loki/config.d.ts
|
Code Review by Qodo
1. Token is now mandatory
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
Comment on lines
40
to
46
| private constructor(config: Config) { | ||
| this.baseURL = config.getString('baseUrl'); | ||
| this.token = config.getString('token'); | ||
| // Only should be false if specified, undefined here should be true | ||
| this.rejectUnauthorized = | ||
| config.getOptionalBoolean('rejectUnauthorized') === false ? false : true; | ||
| this.selectors = config.getOptional('logStreamSelectors') || []; |
There was a problem hiding this comment.
1. Token is now mandatory 🐞 Bug ✓ Correctness
LokiProvider now calls config.getString('token') and always sends a Bearer Authorization header,
causing module initialization to throw if token is missing and breaking previously unauthenticated
Loki deployments. This is particularly risky because the changeset describes this as adding the
ability to pass a token (implying optional), yet the change is released as a patch.
Agent Prompt
### Issue description
`LokiProvider` currently requires `orchestrator.workflowLogProvider.loki.token` via `config.getString('token')`, which throws if the key is missing, breaking existing Loki installations that don’t require auth.
### Issue Context
The changeset describes this as “adds the ability to pass an auth token”, which suggests optional configuration rather than a hard requirement.
### Fix Focus Areas
- workspaces/orchestrator/plugins/orchestrator-backend-module-loki/src/workflowLogsProviders/LokiProvider.ts[40-47]
- workspaces/orchestrator/plugins/orchestrator-backend-module-loki/src/workflowLogsProviders/LokiProvider.ts[136-142]
- workspaces/orchestrator/plugins/orchestrator-backend-module-loki/config.d.ts[23-33]
- workspaces/orchestrator/plugins/orchestrator-backend-module-loki/README.md[9-33]
### Suggested approach
- Change config typing to `token?: string`.
- Read with `getOptionalString('token')`.
- Conditionally add `Authorization` header only when `token` is set (and consider allowing full header value vs forcing Bearer).
- Update README to reflect optional token (or explicitly document breaking change and bump major).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+127
to
+142
| const customAgent = new Agent({ | ||
| connect: { | ||
| // Agent configuration options, e.g., for self-signed certificates | ||
| rejectUnauthorized: this.rejectUnauthorized, | ||
| }, | ||
| }); | ||
|
|
||
| let allResults; | ||
| try { | ||
| const response = await fetch(urlToFetch); | ||
| const response = await fetch(urlToFetch, { | ||
| dispatcher: customAgent, | ||
| headers: { | ||
| Authorization: `Bearer ${this.token}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
2. Agent created per request 🐞 Bug ⛯ Reliability
fetchWorkflowLogsByInstance creates a new undici.Agent on every call and never closes or reuses it, which can leak sockets/handles and degrade performance over time under repeated log queries. This is especially likely because viewing workflow logs from the UI can trigger frequent calls.
Agent Prompt
### Issue description
A new `undici.Agent` is created on every `fetchWorkflowLogsByInstance` call and never closed/reused, risking resource leaks (sockets/handles) and performance degradation.
### Issue Context
`fetchWorkflowLogsByInstance` can be called frequently (e.g., every time a user views logs or refreshes).
### Fix Focus Areas
- workspaces/orchestrator/plugins/orchestrator-backend-module-loki/src/workflowLogsProviders/LokiProvider.ts[124-142]
### Suggested approach
- Prefer: create `private readonly dispatcher: Agent` in the constructor (or lazily on first use) and reuse it.
- Alternative: wrap the fetch in `try/finally` and call `customAgent.close()` (or appropriate teardown) in `finally`.
- Consider only using a custom dispatcher when `baseUrl` is https or when `rejectUnauthorized` is explicitly configured.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
100
to
101
| this.selectors.forEach( | ||
| ( |
There was a problem hiding this comment.
3. Selector default mismatch 🐞 Bug ✓ Correctness
The code now defaults missing selector labels to openshift_log_type, but an existing unit test still expects service_name when a selector entry omits label. This inconsistency will cause the test to fail (and indicates unclear intended behavior for partially specified selectors).
Agent Prompt
### Issue description
Unit tests and implementation disagree about the fallback label when a logStreamSelectors entry omits `label`. Implementation uses `openshift_log_type`, while the test expects `service_name`.
### Issue Context
This is specifically about partially-specified selector entries (e.g., `{ value: '=~".+"' }`).
### Fix Focus Areas
- workspaces/orchestrator/plugins/orchestrator-backend-module-loki/src/workflowLogsProviders/LokiProvider.ts[100-108]
- workspaces/orchestrator/plugins/orchestrator-backend-module-loki/src/workflowLogsProviders/LokiProvider.test.ts[249-297]
### Suggested approach
- Choose one behavior:
- **Option A (OpenShift-first):** keep `openshift_log_type` fallback and update the test’s expected query accordingly.
- **Option B (backward-compatible/generic):** restore fallback to `service_name` and update docs/comments if needed.
- Add/adjust a test to lock in the chosen behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
hopehadfield
force-pushed
the
fix-release-orchestrator
branch
from
March 5, 2026 17:09
300b13c to
ccd1b3c
Compare
|
hopehadfield
changed the title
hopehadfield
merged commit Mar 5, 2026
5a2b6ac
into
redhat-developer:workspace/orchestrator
2 checks passed
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.
Hey, I just made a Pull Request!
Re-pushing this commit along with the workkflow fix to trigger Version Packages again.
✔️ Checklist