feat: Add Jaegar ocp resource#2518
Conversation
WalkthroughAdds a new Jaeger NamespacedResource class for the Jaeger operator CRD with optional strategy, ingress, and collector attributes and a custom to_dict that populates spec when kind_dict/yaml_file are not set. Also adds the JAEGERTRACING_IO API group constant. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ocp_resources/resource.py (1)
504-505: Add explicit type annotation for consistencyMost ApiGroup constants declare
: str. Consider aligningJAEGERTRACING_IOfor consistency and static typing.- JAEGERTRACING_IO = "jaegertracing.io" + JAEGERTRACING_IO: str = "jaegertracing.io"ocp_resources/jaeger.py (2)
15-33: Validate strategy input (early fail on invalid values)Constrain
strategyto accepted values to catch typos at construction time.-from typing import Any +from typing import Any @@ def __init__( @@ - super().__init__(**kwargs) + super().__init__(**kwargs) + + allowed_strategies = {"allInOne", "production"} + if strategy is not None and strategy not in allowed_strategies: + raise ValueError(f"Invalid strategy '{strategy}'. Allowed: {sorted(allowed_strategies)}")Optionally, tighten the type hint:
-from typing import Any +from typing import Any, Literal @@ - strategy: str | None = None, + strategy: Literal["allInOne", "production"] | None = None,
34-50: Don’t wipe existing spec on repeated to_dict() callsResetting
self.res["spec"] = {}can drop prior fields ifto_dict()is called more than once. Prefer merging and only overriding when arguments are provided.- if not self.kind_dict and not self.yaml_file: - self.res["spec"] = {} - _spec = self.res["spec"] - - # If no strategy is set, default to "allInOne" - _spec["strategy"] = self.strategy or "allInOne" - - if self.ingress is not None: - _spec["ingress"] = self.ingress - - if self.collector is not None: - _spec["collector"] = self.collector + if not self.kind_dict and not self.yaml_file: + _spec = self.res.setdefault("spec", {}) + + if self.strategy is not None: + _spec["strategy"] = self.strategy + else: + _spec.setdefault("strategy", "allInOne") + + if self.ingress is not None: + _spec["ingress"] = self.ingress + + if self.collector is not None: + _spec["collector"] = self.collector
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ocp_resources/jaeger.py(1 hunks)ocp_resources/resource.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ocp_resources/jaeger.py (1)
ocp_resources/resource.py (4)
NamespacedResource(1532-1642)ApiGroup(466-574)to_dict(735-739)to_dict(1640-1642)
🔇 Additional comments (1)
ocp_resources/jaeger.py (1)
13-13: LGTM: correct API group referenceUsing
NamespacedResource.ApiGroup.JAEGERTRACING_IOmatches the Jaeger operator CRD group.
|
Hi @adolfo-ab @myakove @rnetser please review this PR, Thanks! |
|
/retest all |
new file: ocp_resources/jaeger.py modified: ocp_resources/resource.py new file: ocp_resources/jaeger.py modified: ocp_resources/resource.py
kpunwatk
force-pushed
the
opentelemetry_resource
branch
from
36ceae8 to
288ef40
Compare
|
/approve |
|
/verified |
5 participants
Adds jaegar resource to the wrapper
Short description:
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Bug:
Summary by CodeRabbit