Create README.md#97
Conversation
Farjaad
left a comment
There was a problem hiding this comment.
Overall
The PR adds a README for a PagerDuty Service ID mapping integration. The content is accurate and useful, and it follows the established pattern from FireHydrant and Rootly. A few issues worth addressing:
Issues
Functional / Correctness
1. Pagination is not handled
The extractor URL uses ?limit=100 but PagerDuty's API paginates using offset and more fields. If an account has more than 100 services, the integration will silently truncate. Either document this limitation explicitly, or show how to handle it (e.g., using a higher limit like limit=500, which is PagerDuty's max).
2. opslevel_identifier matching assumption is fragile
opslevel_identifier: .name | ascii_downcase | gsub(" "; "-")This transformation is a guess — it only works if OpsLevel aliases happen to match this exact slug format. Many services will have aliases that differ (e.g. underscores, different casing schemes, or entirely different names). The doc acknowledges this in prose but should more prominently recommend users verify their alias format before relying on this.
Structural / Style
3. Step 1 formatting is inconsistent with other READMEs
FireHydrant and Rootly use a numbered list under Step 1. This PR uses a bare **Define Custom Properties**: line without a numbered list wrapper, which renders oddly (the image sits directly under the bold label with no list structure).
4. Step 4 YAML indentation differs from repo conventions
Other READMEs indent the YAML block inside a numbered list with 4 spaces. This PR's YAML is at the top level (no indentation), which is inconsistent — minor but visible.
5. Step 4 missing iterator explanation line
The FireHydrant README explains iterator under the YAML bullet points. This PR explains the other fields but skips a note clarifying that .services is PagerDuty-specific (the response wraps results in a services key), which is non-obvious to new users.
6. No --- separator / "Important Considerations" section
Rootly includes a horizontal rule and an "Important Considerations" section covering JQ expressions and filtering. This PR omits any equivalent, even though pagination and alias-matching are exactly the kinds of things that section would cover.
Minor / Nits
- Step 7's
defaultValueExpressionuses.service.properties.pd_service_id— worth confirming this is the exact property key path the custom actions runtime resolves (the property identifier defined in Step 1 ispd_service_id, which looks correct, but worth a double-check given property identifiers are auto-generated). - The PR title is just "Create README.md" — not meaningful in the context of the repo's commit history.
- No description in the PR body.
Adds a community example for syncing PagerDuty Service IDs into OpsLevel custom properties via a pull-based Custom Integration. Enables Custom Actions (e.g. triggering PagerDuty incidents) to reference the Service ID automatically without manual tagging.
|
@Farjaad thank you for your suggestions! I have made relevant changes, could you please review them? |
Farjaad
left a comment
There was a problem hiding this comment.
A few remaining issues after the latest update:
1. URL still says limit=100, not limit=500
The bullet point below the YAML correctly mentions limit=500 as the max, but the actual YAML config still has ?limit=100:
url: https://api.pagerduty.com/services?limit=100This is contradictory — the config will truncate at 100 while the note says to use 500. The URL in the YAML should be updated to ?limit=500.
2. YAML indentation bug in the extractor
There's an extra space before value on the Accept header line:
- name: Accept
value: application/vnd.pagerduty+json;version=2This is misaligned with the Authorization header's value and would cause a YAML parse error if copied verbatim. Should be:
- name: Accept
value: application/vnd.pagerduty+json;version=23. "Important Considerations" interrupts the numbered step flow
Steps jump from 5 → Important Considerations → Step 6 → Step 7. The section belongs at the end after Step 7 (consistent with how Rootly does it), not in the middle of the numbered sequence.
|
@Farjaad good to go now! |
No description provided.