Plugin ips rest#1
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a complete Atlassian JIRA plugin that exposes IPS (Infrastructure Platform Services) Release requirement data via REST API. The implementation spans Maven multi-module project structure, Feign-based HTTP client library, core IPS data aggregation service, admin configuration UI, GitHub CI/CD pipelines, and comprehensive test coverage. ChangesIPS JIRA API Plugin
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yml (1)
1-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin the reusable workflow to a commit SHA instead of a mutable version tag, and add explicit token permissions.
The workflow currently references the reusable workflow via version tag
@v2.3.4, which is mutable and can be moved or deleted. GitHub security best practices recommend pinning to a full commit SHA for immutability. Additionally, explicit permissions should be set to follow the principle of least privilege.🔧 Proposed fix
name: Maven Compile & UT on: [push, pull_request, workflow_dispatch] +permissions: + contents: read + jobs: run-build-and-deploy: - uses: octopusden/octopus-base/.github/workflows/common-java-maven-build.yml@v2.3.4 + uses: octopusden/octopus-base/.github/workflows/common-java-maven-build.yml@<full-commit-sha> with: java-version: '8'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml around lines 1 - 10, Update the reusable workflow invocation by replacing the mutable tag on the `uses: octopusden/octopus-base/.github/workflows/common-java-maven-build.yml@v2.3.4` with the full commit SHA for immutability, and add an explicit top-level `permissions` block to the workflow (least-privilege, e.g., `contents: read` and any other minimal permissions required by the reused workflow) so the `run-build-and-deploy` job runs with explicit token scopes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/check-and-register.yml:
- Around line 4-7: The workflow currently uses workflow_run with secrets:
inherit without sufficient validation; add trust-boundary guards on the
consuming job (e.g., the job named build) by adding an if condition that
validates the upstream workflow name, repository and branch before inheriting
secrets — for example use github.event.workflow_run.name (or workflow) == "Maven
Release" AND github.event.workflow_run.head_repository.full_name ==
github.repository AND github.event.workflow_run.head_branch == 'main' AND
github.event.workflow_run.conclusion == 'success' (or
github.event.workflow_run.status/type checks) so the job (build) only runs and
inherits secrets when all these checks pass, and remove/avoid secrets: inherit
unless those guards are present.
In @.github/workflows/release.yml:
- Line 16: Replace the blanket "secrets: inherit" in the release job call to the
external workflow (the "uses:
octopusden/octopus-base/.github/workflows/common-java-maven-release.yml@v2.3.4"
invocation) by listing only the specific secrets that workflow needs (e.g.,
OCTOPUS_API_KEY, MAVEN_CENTRAL_USERNAME, MAVEN_CENTRAL_PASSWORD or whichever the
external workflow documents). Edit the job's "secrets:" mapping to explicitly
map each secret name to a repository secret (e.g., secrets: { OCTOPUS_API_KEY:
${{ secrets.OCTOPUS_API_KEY }}, ... }), removing "inherit" entirely.
- Around line 1-12: Add an explicit top-level permissions block and SHA-pin the
reusable workflow reference: replace the mutable tag in the uses value
"octopusden/octopus-base/.github/workflows/common-java-maven-release.yml@v2.3.4"
with the full commit SHA (you may keep a trailing comment with "v2.3.4" for
clarity) and add a top-level permissions: map granting only required scopes for
this job (e.g., contents: read, id-token: write, packages: read — adjust to the
actual needs) so GITHUB_TOKEN follows least-privilege rules.
In `@doc/octopus-jira-api-plugin.adoc`:
- Line 10: Replace the broken sentence "Octopus Jira API Plugin is specific
Octopus API." with a clear user-facing description such as "The Octopus Jira API
Plugin integrates Jira with the Octopus API." and replace the non-actionable
placeholders since={{}} and sinceDate={{}} with concrete, actionable examples or
templated tokens (e.g., since=VERSION or sinceDate=YYYY-MM-DD) wherever they
appear (also update the other occurrences noted around the same section).
- Line 16: The documented GET path shown in doc/octopus-jira-api-plugin.adoc
omits the required /type/{{type}} segment; update the example request path (the
string starting with "GET
{{jira-url}}/rest/octopus-jira-api/1/api/ips/{{ips}}?") to include
"/type/{{type}}" (i.e., "GET
{{jira-url}}/rest/octopus-jira-api/1/api/ips/{{ips}}/type/{{type}}?") so the
sample matches the API contract and routes correctly.
In `@pom.xml`:
- Around line 282-287: Update the Maven repository checksumPolicy settings to
fail rather than warn: locate the <checksumPolicy> elements inside both
<releases> and <snapshots> blocks for the repository and pluginRepository
entries and change their values from "warn" to "fail" so checksum mismatches
abort the build; ensure you update all occurrences of <checksumPolicy> within
those <releases> and <snapshots> sections in the POM.
In
`@src/main/java/org/octopusden/octopus/jira/api/ui/admin/AbstractViewApiSettings.java`:
- Around line 41-44: The current code in AbstractViewApiSettings reads
request.getParameter(setting.getKey()) and persists any non-null value, allowing
empty strings to overwrite sensitive settings like SERVICE_USER_PASSWORD; change
the logic so you only call settingsProvider.set(setting, value) when value is
non-null and not blank (e.g., value.trim().isEmpty() == false), and for the
specific sensitive key (SERVICE_USER_PASSWORD) treat blank values as "do not
change" so submissions that omit or send an empty password will not overwrite
the stored credential; update the condition around request.getParameter(...) and
settingsProvider.set(...) to enforce this check for both the general case and
the SERVICE_USER_PASSWORD identifier.
- Around line 38-46: The doDefault() method currently mutates settings on GET;
move the persistence logic into a new doExecute() method so doDefault() only
prepares the view, and perform explicit CSRF validation in doExecute() by
reading the atl_token request parameter (via ExecutingHttpRequest.get()) and
validating it before calling settingsProvider.set(...) for each ApiSetting from
getAvailableSettings(); keep getAvailableSettings(), settingsProvider and
ApiSetting.key usage but ensure doExecute() returns SUCCESS only after CSRF
passes and settings are persisted, otherwise return an error/forbidden result.
In
`@src/main/kotlin/org/octopusden/octopus/jira/api/config/ApiSettingsProvider.kt`:
- Around line 32-34: The getString function currently treats empty strings as
valid; update the ApiSettingsProvider.getString implementation so that after
retrieving the value via get(key) it not only checks for null but also rejects
blank/empty strings (use String.isBlank()) and throws the same
IllegalStateException with context (including key.key) when the value is blank;
ensure this change is applied only to getString so other getters' behavior is
unchanged.
- Around line 14-17: The resource InputStream returned by
ApiSettingsProvider::class.java.getResourceAsStream(PROPERTIES_FILE_NAME) must
be null-checked before calling .use() to avoid a NullPointerException during
class init; change the block so you retrieve the stream into a variable, if the
stream is null throw a clear IllegalStateException (or similar) mentioning
PROPERTIES_FILE_NAME and that the config is missing, otherwise call stream.use {
load(stream) } so load(stream) only runs with a non-null stream.
- Around line 23-25: The setter currently writes all values (including
passwords) with pluginSettings.put in operator fun set(setting: ApiSetting,
value: String?), so replace plain-text storage for
ApiSetting.SERVICE_USER_PASSWORD with the Atlassian Plugin Secrets Service:
inject PluginSecretsService into this class, and in operator fun set use
PluginSecretsService (e.g., its secret store API) to persist
SERVICE_USER_PASSWORD while keeping pluginSettings.put for non-secret settings;
also update corresponding getter logic that reads SERVICE_USER_PASSWORD to
retrieve from PluginSecretsService instead of pluginSettings.
In `@src/main/kotlin/org/octopusden/octopus/jira/api/rest/ApiRestService.kt`:
- Around line 90-93: The catch block in ApiRestService (the exception handler
around the IPS generation call) currently returns the raw exception message to
the client; update the Response.serverError() entity to return a generic payload
(e.g., {"error":"Internal server error"}) and remove e.message from the HTTP
response while keeping the detailed error in the server log via
logger.error("Failed to generate IPS data for
${request.ips}:${request.release}", e); ensure the server response does not
expose internal details from the caught Exception.
- Around line 37-38: The route template includes an unused {type} and the code
double-decodes the ips path param; update the `@Path` on getIPS (method getIPS in
ApiRestService) to remove "/type/{type}" so it reads "ips/{ips}", and stop
calling URLDecoder.decode(ips, "UTF-8") when constructing IPSRequest (both
places where IPSRequest(ips = URLDecoder.decode(ips, ...)) is used); pass the
JAX-RS-provided ips value directly to IPSRequest and remove the URLDecoder
import/usages related to this decoding.
- Around line 23-24: The ApiRestService class is currently annotated with
`@AnonymousAllowed` which permits unauthenticated access to sensitive JIRA issue
metadata; remove the `@AnonymousAllowed` annotation from the class or replace it
with an authentication requirement and/or add explicit per-request authorization
checks inside the ApiRestService endpoint methods (e.g., methods handling issue
retrieval) to validate the caller’s identity and permissions before returning
requirements, subtasks, components, versions, or custom fields; ensure you use
your platform’s authentication/authorization utilities (or security annotations)
consistently across each public method in ApiRestService to enforce access
control.
In `@src/main/kotlin/org/octopusden/octopus/jira/api/service/IPSService.kt`:
- Around line 61-65: The response currently sets release = request.release ?: ""
which yields an empty string for date-based requests; change it to prefer
request.release but fall back to the resolved IPS issue's release (e.g. release
= request.release ?: ipsReleaseIssue.release ?: "") so the IPSResponse includes
the concrete release value discovered on ipsReleaseIssue; update the IPSResponse
construction in the code that builds the response (look for IPSResponse(...)
near ipsReleaseIssue and request.release) to use this fallback.
In `@src/main/resources/octopus-jira-api-plugin.properties`:
- Around line 2-3: Remove the hardcoded default credentials by deleting the
literal values for service-user.username and service-user.password and instead
read them from a secure source (e.g., environment variables or a
secret-management provider) at runtime; update the code/config that consumes
these keys to expect externalized values (service-user.username,
service-user.password) and fail fast with a clear error if they are missing, and
add documentation or a sample .env.example showing placeholder names rather than
real credentials.
In `@src/main/resources/templates/ui/admin/settings-view.vm`:
- Around line 4-6: The template is rendering unescaped setting values into input
value attributes (via $settingsProvider.get($setting) and name="$setting.key"),
creating a stored XSS risk; update the Velocity template to HTML-escape the
setting key and value before output (use the Velocity escape tool or an existing
escape utility, e.g., $esc.html($settingsProvider.get($setting)) and
$esc.html($setting.key) or the project’s equivalent) so both the name and value
attributes are safely encoded when rendered in settings-view.vm.
- Around line 3-4: The password input is pre-populating the persisted secret by
using settingsProvider.get($setting) in the value attribute; remove the value
population for type PASSWORD inputs so the <input name="$setting.key"
type="password"> is rendered without a value (or explicitly render an empty
value) and rely on server-side update handling to only change the stored secret
when a non-empty password is submitted; update the template branch that checks
$setting.type.name() == "PASSWORD" and remove or replace the
value="$settingsProvider.get($setting)" usage to prevent exposing secrets in the
DOM.
In `@src/test/kotlin/org/octopusden/octopus/jira/api/service/IPSServiceTest.kt`:
- Around line 173-216: Add a unit test that asserts service.generate(request)
throws when the "System" custom field is missing: mock
customFieldManager.getCustomFieldObjectsByName("System") to return an empty
list, call createRequest() and invoke service.generate(request) expecting an
IllegalStateException, and assert the exception message contains "System"; name
the test testGenerateThrowsWhenSystemCustomFieldNotFound to match the existing
style and place it alongside the other missing-custom-field tests.
- Around line 82-90: The mocked JQL builder setup uses relaxed mocks which
prevents verifying that the correct query clauses are invoked; change the mocks
for JqlClauseBuilder (mockWhereBuilder) and/or mockQueryBuilder to non-relaxed
where needed and add verify blocks in the relevant tests (e.g.,
testGenerateWithReleaseParamBuildsJqlWithIpsReleaseField and the startDate path
test) to assert calls on mockWhereBuilder such as project(IPS_PROJECT),
customField(10002L).eq(RELEASE) for the release path and the appropriate
customField(...).ge(...) / date filter calls for the startDate path, and also
verify JqlQueryBuilder.newBuilder().where() was used to ensure the builder flow
is executed.
---
Outside diff comments:
In @.github/workflows/build.yml:
- Around line 1-10: Update the reusable workflow invocation by replacing the
mutable tag on the `uses:
octopusden/octopus-base/.github/workflows/common-java-maven-build.yml@v2.3.4`
with the full commit SHA for immutability, and add an explicit top-level
`permissions` block to the workflow (least-privilege, e.g., `contents: read` and
any other minimal permissions required by the reused workflow) so the
`run-build-and-deploy` job runs with explicit token scopes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38ccc03a-3135-4d0f-9bfe-57377970865f
⛔ Files ignored due to path filters (2)
src/main/resources/images/pluginIcon.pngis excluded by!**/*.pngsrc/main/resources/images/pluginLogo.pngis excluded by!**/*.png
📒 Files selected for processing (22)
.github/release.yml.github/workflows/build.yml.github/workflows/check-and-register.yml.github/workflows/release.yml.gitignoredoc/octopus-jira-api-plugin.adocpom.xmlsrc/main/java/org/octopusden/octopus/jira/api/ui/admin/AbstractViewApiSettings.javasrc/main/java/org/octopusden/octopus/jira/api/ui/admin/ViewApiSettings.javasrc/main/kotlin/org/octopusden/octopus/jira/api/config/ApiSettingsProvider.ktsrc/main/kotlin/org/octopusden/octopus/jira/api/dto/DevComponent.ktsrc/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSReqDev.ktsrc/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSReqQA.ktsrc/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSRequirement.ktsrc/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSResponse.ktsrc/main/kotlin/org/octopusden/octopus/jira/api/dto/IssueBean.ktsrc/main/kotlin/org/octopusden/octopus/jira/api/rest/ApiRestService.ktsrc/main/kotlin/org/octopusden/octopus/jira/api/service/IPSService.ktsrc/main/resources/atlassian-plugin.xmlsrc/main/resources/octopus-jira-api-plugin.propertiessrc/main/resources/templates/ui/admin/settings-view.vmsrc/test/kotlin/org/octopusden/octopus/jira/api/service/IPSServiceTest.kt
There was a problem hiding this comment.
♻️ Duplicate comments (1)
doc/octopus-jira-api-plugin.adoc (1)
16-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument endpoint path must include the required
typesegment.Line 16 still omits
/type/{{type}}, so the sample request does not match the REST route contract and can lead clients to a non-existent path.🔧 Proposed fix
-GET {{jira-url}}/rest/octopus-jira-api/1/api/ips/{{ips}}? +GET {{jira-url}}/rest/octopus-jira-api/1/api/ips/{{ips}}/type/{{type}}?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/octopus-jira-api-plugin.adoc` at line 16, The example endpoint path is missing the required path segment /type/{{type}}; update the sample GET URL (the string starting "GET {{jira-url}}/rest/octopus-jira-api/1/api/ips/{{ips}}?") to include /type/{{type}} immediately after {{ips}} (i.e. ".../ips/{{ips}}/type/{{type}}?...") and ensure any trailing query parameters remain intact so the documented request matches the REST route contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@doc/octopus-jira-api-plugin.adoc`:
- Line 16: The example endpoint path is missing the required path segment
/type/{{type}}; update the sample GET URL (the string starting "GET
{{jira-url}}/rest/octopus-jira-api/1/api/ips/{{ips}}?") to include
/type/{{type}} immediately after {{ips}} (i.e.
".../ips/{{ips}}/type/{{type}}?...") and ensure any trailing query parameters
remain intact so the documented request matches the REST route contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a547658-0250-4f44-a4b0-772d9cf27963
📒 Files selected for processing (3)
doc/octopus-jira-api-plugin.adocpom.xmlsrc/main/kotlin/org/octopusden/octopus/jira/api/rest/ApiRestService.kt
rodemark
reviewed
May 21, 2026
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/src/main/kotlin/org/octopusden/octopus/jira/api/rest/ApiRestService.kt (1)
59-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
since(year-only) parsing inApiRestService.getIPS()
sinceYearusesLocalDate.parse(sinceYear.toString(), DATE_FORMAT_YEAR)withDATE_FORMAT_YEAR = yyyy, which fails for inputs like2026(year-only strings aren’t enough to construct aLocalDate). Build the date explicitly instead.
Also, diagnostics are weakened:logger.error(it)logs only a string, andFailedGenerateIPSException(message: String)doesn’t preserve the original throwable/cause.Proposed fix
- sinceYear != null -> Date.from( - LocalDate.parse(sinceYear.toString(), DATE_FORMAT_YEAR) - .atStartOfDay().toInstant(ZoneOffset.UTC) - ) + sinceYear != null -> Date.from( + LocalDate.of(sinceYear, 1, 1) + .atStartOfDay().toInstant(ZoneOffset.UTC) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/src/main/kotlin/org/octopusden/octopus/jira/api/rest/ApiRestService.kt` around lines 59 - 61, In ApiRestService.getIPS() fix year-only parsing by treating sinceYear as an integer/Year and constructing a LocalDate via LocalDate.of(year, 1, 1) (or Year.of(year).atDay(1)) then convert to Instant at UTC instead of LocalDate.parse(..., DATE_FORMAT_YEAR); also replace logger.error(it) usage to log the throwable and message (e.g., logger.error("...", it)) and throw FailedGenerateIPSException with the original throwable as the cause (use the constructor or add a cause parameter) so the original exception is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@client/src/main/kotlin/org/octopusden/octopus/jira/api/client/JiraApiClientErrorDecoder.kt`:
- Around line 12-24: The decode implementation in JiraApiClientErrorDecoder
currently consumes the response InputStream and does a case-sensitive headers
lookup, which prevents ErrorDecoder.Default from seeing the original body and
misses differently-cased Content-Type headers; fix it by reading and buffering
the response body into a String (e.g., responseBody), perform a case-insensitive
check for Content-Type by iterating response.headers() keys or normalizing
header names, attempt JSON parsing from the buffered responseBody as before, and
if parsing fails or Content-Type doesn't indicate JSON, recreate or wrap the
original Response with the buffered body intact and pass that Response into
super.decode(methodKey, preservedResponse) so fallback decoding can see the
unconsumed payload (refer to the decode method and variable names responseBody,
objectMapper, ApplicationErrorResponse, JiraApiException).
In `@plugin/pom.xml`:
- Around line 209-215: The maven-enforcer-plugin configuration currently
disables all enforcement via the <fail>false</fail> flag; revert this by
removing or setting <fail>true</fail> and instead selectively disable only the
problematic rules inside the maven-enforcer-plugin configuration (e.g.,
configure specific rule entries like dependencyConvergence, requireMavenVersion,
bannedDependencies with <enabled>false</enabled> or rule-specific <excludes>),
so enforcement remains active for other checks while skipping only the
identified failing rule(s) referenced in the plugin configuration.
- Around line 160-164: The Kotlin test compilation's <sourceDirs> currently
includes a resource folder which causes the Kotlin compiler to parse non-Kotlin
files; remove the <sourceDir>${project.basedir}/src/test/resources</sourceDir>
entry from the <sourceDirs> block so only Kotlin/Java test source directories
remain (leave ${project.basedir}/src/test/kotlin and
${project.basedir}/src/test/java), ensuring test resources are handled by the
resources phase rather than the Kotlin compiler.
- Around line 145-149: Remove the resources entry from the Kotlin compilation
sourceDirs so the Kotlin compiler doesn't try to parse non‑Kotlin files: edit
the <sourceDirs> block and delete the
<sourceDir>${project.basedir}/src/main/resources</sourceDir> line (leave the
Kotlin and Java sourceDir entries intact) so only actual source folders are
provided to the Kotlin compiler (refer to the <sourceDirs> / <sourceDir>
elements in the pom.xml).
- Around line 191-208: The build-helper-maven-plugin declaration incorrectly
uses the Maven runtime property ${maven.version} as its plugin version; update
the <plugin> block for build-helper-maven-plugin so it either specifies a valid
build-helper-maven-plugin version (e.g., a known release) instead of
${maven.version}, or remove the <version> element so the version is inherited
from pluginManagement; adjust the <plugin> element containing
<groupId>org.codehaus.mojo</groupId> and
<artifactId>build-helper-maven-plugin</artifactId> accordingly.
In
`@plugin/src/main/kotlin/org/octopusden/octopus/jira/api/rest/ApiRestService.kt`:
- Around line 91-93: The current code in ApiRestService builds a message string
including ${e} but calls logger.error(it) so the exception stack trace is lost;
update the logging call to pass the throwable (use logger.error(message, e)) and
when throwing FailedGenerateIPSException preserve the original cause (throw a
FailedGenerateIPSException(message, e) or otherwise attach e as the cause)
referencing logger.error, FailedGenerateIPSException, ApiRestService, and the
request.ips/request.release context so the stack trace and root cause are
preserved.
In `@pom.xml`:
- Line 61: The pom pins an insecure Jackson version via the property
jackson.version (currently 2.9.2) and an outdated SLF4J via slf4j.version;
update jackson.version to a supported, non-vulnerable release (e.g., bump to a
current 2.x LTS such as 2.15.x or at minimum 2.12+ depending on compatibility)
and update slf4j.version to a modern release (e.g., 1.7.36 or 2.x) to remediate
CVEs; locate the jackson.version and slf4j.version properties in the POM and
change their values, then run mvn dependency:tree and your test suite to verify
there are no breaking changes and adjust jackson-module-kotlin or other
Jackson-bound artifacts if API/compatibility changes require minor code updates.
---
Outside diff comments:
In
`@plugin/src/main/kotlin/org/octopusden/octopus/jira/api/rest/ApiRestService.kt`:
- Around line 59-61: In ApiRestService.getIPS() fix year-only parsing by
treating sinceYear as an integer/Year and constructing a LocalDate via
LocalDate.of(year, 1, 1) (or Year.of(year).atDay(1)) then convert to Instant at
UTC instead of LocalDate.parse(..., DATE_FORMAT_YEAR); also replace
logger.error(it) usage to log the throwable and message (e.g.,
logger.error("...", it)) and throw FailedGenerateIPSException with the original
throwable as the cause (use the constructor or add a cause parameter) so the
original exception is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea5a0c13-0cff-4dad-ad81-460f880d0bac
⛔ Files ignored due to path filters (2)
plugin/src/main/resources/images/pluginIcon.pngis excluded by!**/*.pngplugin/src/main/resources/images/pluginLogo.pngis excluded by!**/*.png
📒 Files selected for processing (29)
client/pom.xmlclient/src/main/kotlin/org/octopusden/octopus/jira/api/client/JiraApiClient.ktclient/src/main/kotlin/org/octopusden/octopus/jira/api/client/JiraApiClientErrorDecoder.ktclient/src/main/kotlin/org/octopusden/octopus/jira/api/client/impl/ClassicJiraApiClient.ktclient/src/main/kotlin/org/octopusden/octopus/jira/api/client/impl/JiraApiClientParametersProvider.ktclient/src/test/kotlin/org/octopusden/octopus/jira/api/client/JiraApiClientErrorDecoderTest.ktclient/src/test/kotlin/org/octopusden/octopus/jira/api/client/impl/ClassicJiraApiClientTest.ktcommon/pom.xmlcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/DevComponent.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSReqDev.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSReqQA.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSRequest.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSRequirement.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSResponse.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IssueBean.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/exception/JiraApiException.ktplugin/doc/octopus-jira-api-plugin.adocplugin/pom.xmlplugin/src/main/java/org/octopusden/octopus/jira/api/ui/admin/AbstractViewApiSettings.javaplugin/src/main/java/org/octopusden/octopus/jira/api/ui/admin/ViewApiSettings.javaplugin/src/main/kotlin/org/octopusden/octopus/jira/api/config/ApiSettingsProvider.ktplugin/src/main/kotlin/org/octopusden/octopus/jira/api/exception/JiraApiExceptionMapper.ktplugin/src/main/kotlin/org/octopusden/octopus/jira/api/rest/ApiRestService.ktplugin/src/main/kotlin/org/octopusden/octopus/jira/api/service/IPSService.ktplugin/src/main/resources/atlassian-plugin.xmlplugin/src/main/resources/octopus-jira-api-plugin.propertiesplugin/src/main/resources/templates/ui/admin/settings-view.vmplugin/src/test/kotlin/org/octopusden/octopus/jira/api/service/IPSServiceTest.ktpom.xml
💤 Files with no reviewable changes (12)
- common/src/main/kotlin/org/octopusden/octopus/jira/api/dto/DevComponent.kt
- plugin/src/main/resources/octopus-jira-api-plugin.properties
- plugin/doc/octopus-jira-api-plugin.adoc
- common/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSReqQA.kt
- plugin/src/main/resources/templates/ui/admin/settings-view.vm
- common/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSResponse.kt
- plugin/src/main/java/org/octopusden/octopus/jira/api/ui/admin/ViewApiSettings.java
- common/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSReqDev.kt
- common/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSRequirement.kt
- common/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IssueBean.kt
- plugin/src/main/java/org/octopusden/octopus/jira/api/ui/admin/AbstractViewApiSettings.java
- plugin/src/main/kotlin/org/octopusden/octopus/jira/api/config/ApiSettingsProvider.kt
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugin/src/main/kotlin/org/octopusden/octopus/jira/api/service/IPSService.kt (1)
92-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPopulate
IPSResponse.releasefrom the resolved issue when request release is absent.Line 95 still returns an empty release for date-based requests, even when a concrete IPS Release issue was found. This drops response data.
Proposed fix
return IPSResponse( key = ipsReleaseIssue.key, ips = request.ips, - release = request.release ?: "", + release = request.release ?: getCustomFieldStringValue(fieldIpsRelease, ipsReleaseIssue), status = ipsReleaseIssue.status.name, labels = ipsReleaseIssue.labels.map { it.label }, requirements = requirements )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/src/main/kotlin/org/octopusden/octopus/jira/api/service/IPSService.kt` around lines 92 - 99, The IPSResponse currently sets release to request.release ?: "" which leaves it empty for date-based requests even when a concrete IPS release issue was resolved; change the assignment to use the resolved IPS release issue's release value when request.release is null (e.g., release = request.release ?: ipsReleaseIssue.release or the appropriate property on ipsReleaseIssue), leaving other fields (key, status, labels, requirements) unchanged; update the return in the code that builds IPSResponse (the IPSResponse constructor usage where ipsReleaseIssue and request are available) to populate release from ipsReleaseIssue when request.release is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugin/src/main/kotlin/org/octopusden/octopus/jira/api/service/IPSService.kt`:
- Around line 118-125: The mapping over versionToIssues produces duplicate
DevComponent.issues when the same issue appears with multiple fix versions;
after grouping into DevComponent (in the lambda that builds DevComponent for
each compName), change the issues population to remove duplicates (e.g., collect
pairs.map { it.second } then call distinct()/toSet().toList() or distinctBy(...)
as appropriate) so that DevComponent.fixVersions remains the aggregated list
while DevComponent.issues contains only unique issues.
---
Duplicate comments:
In
`@plugin/src/main/kotlin/org/octopusden/octopus/jira/api/service/IPSService.kt`:
- Around line 92-99: The IPSResponse currently sets release to request.release
?: "" which leaves it empty for date-based requests even when a concrete IPS
release issue was resolved; change the assignment to use the resolved IPS
release issue's release value when request.release is null (e.g., release =
request.release ?: ipsReleaseIssue.release or the appropriate property on
ipsReleaseIssue), leaving other fields (key, status, labels, requirements)
unchanged; update the return in the code that builds IPSResponse (the
IPSResponse constructor usage where ipsReleaseIssue and request are available)
to populate release from ipsReleaseIssue when request.release is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fcd8711-2aee-464d-a841-19a6e969bd34
📒 Files selected for processing (7)
common/src/main/kotlin/org/octopusden/octopus/jira/api/dto/DevComponent.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSReqDev.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSReqQA.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSRequirement.ktcommon/src/main/kotlin/org/octopusden/octopus/jira/api/dto/IPSResponse.ktplugin/src/main/kotlin/org/octopusden/octopus/jira/api/rest/ApiRestController.ktplugin/src/main/kotlin/org/octopusden/octopus/jira/api/service/IPSService.kt
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
pgorbachev
reviewed
May 22, 2026
pgorbachev
left a comment
pgorbachev
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the quick turnaround on the previous round — most things look great. A couple of small follow-ups below.
| } | ||
| } catch (e: Exception) { | ||
| "Failed to generate IPS data for ${request.ips}:${request.release} ${e}".let { | ||
| logger.error(it) |
Collaborator
There was a problem hiding this comment.
Small follow-up — I don't think this one was addressed yet (no reply on the original thread, so flagging again in case it slipped):
logger.error(it) takes only a string, so the Throwable is dropped on the floor and the stack trace never reaches the logs. The two-arg form keeps the cause intact:
catch (e: Exception) {
logger.error("Failed to generate IPS data for ${request.ips}:${request.release}", e)
throw FailedGenerateIPSException("Failed to generate IPS data: ${e.message}")
}Not a merge blocker for me, but it'll save us from having to reproduce issues in dev just to see the stack.
Collaborator
Author
There was a problem hiding this comment.
Obsolete
now
} catch (e: Exception) {
"Failed to generate IPS data for ${request.ips}:${request.release} ${e}".let {
logger.error(it)
throw FailedGenerateIPSException(it)
}
rodemark
previously approved these changes
May 22, 2026
pgorbachev
previously approved these changes
May 22, 2026
pgorbachev
left a comment
pgorbachev
left a comment
Collaborator
There was a problem hiding this comment.
Approving — the bulk of the review feedback is addressed and CI is green. Nice work on the JacksonMapper extraction and the test refactor for the cached custom fields.
Two small items from my last comment can land as a follow-up PR (no need to hold this one):
ApiRestController.kt:74— the secondURLDecoder.decode(...)call in theelsebranch (one-line fix, matches the change already done on line 51)ApiRestController.kt:93—logger.error(it)losing theThrowable(two-arg form to preserve the stack trace)
Both are isolated and safe to do separately whenever you have a few minutes.
pgorbachev
reviewed
May 22, 2026
pgorbachev
left a comment
pgorbachev
left a comment
Collaborator
There was a problem hiding this comment.
Two small things from the latest commit:
| private final List<ApiSetting> availableSettings = Arrays.asList( | ||
| ApiSetting.IPS_REPORTS_PROJECT, | ||
| ApiSetting.SERVICE_USER, | ||
| ApiSetting.SERVICE_USER_PASSWORD |
Collaborator
There was a problem hiding this comment.
CI is red on this — removing SERVICE_USER_PASSWORD from the ApiSetting enum left this Java reference dangling:
[ERROR] ViewApiSettings.java:[14,23] cannot find symbol
[ERROR] symbol: variable SERVICE_USER_PASSWORD
[ERROR] location: class org.octopusden.octopus.jira.api.config.ApiSetting
Just drop this line and the trailing comma on line 13, should be a one-character compile fix.
| } | ||
| } catch (e: Exception) { | ||
| "Failed to generate IPS data for ${request.ips}:${request.release} ${e}".let { | ||
| logger.error(it) |
Collaborator
There was a problem hiding this comment.
I might be missing something, but I think the "obsolete now" reply may have been a confusion — the catch (e: Exception) block on lines 90–95 wasn't actually touched in 9d2e4d8f, so the snippet you quoted as obsolete is exactly what's still in HEAD:
} catch (e: Exception) {
"Failed to generate IPS data for ${request.ips}:${request.release} ${e}".let {
logger.error(it)
throw FailedGenerateIPSException(it)
}
}If you had a different change in mind that I overlooked — happy to take another look. Otherwise the two-arg logger.error(msg, e) form (and dropping ${e} from the message that goes to the client) still applies.
rodemark
approved these changes
May 22, 2026
pgorbachev
approved these changes
May 22, 2026
pgorbachev
left a comment
pgorbachev
left a comment
Collaborator
There was a problem hiding this comment.
Re-approving after the latest fixes — 1abe50e9 cleared the compile error and CI is green again. Apologies for my "CI is red" note 8 minutes after you'd already fixed it.
logger.error on lines 90–95 is the only loose end from my side, and as I mentioned it's not a blocker — happy for it to land as a follow-up whenever convenient.
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
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.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores