andreahlert commented on code in PR #345:
URL: https://github.com/apache/airflow-steward/pull/345#discussion_r3318501169
##########
tools/jira/bridge.groovy:
##########
@@ -130,20 +202,172 @@ def cmd_projects(List args) {
])
}
+def cmd_comment(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ String bodyFile = null
+ def i = 1
+ while (i < args.size()) {
+ if (args[i] == '--body-file' && i + 1 < args.size()) {
+ bodyFile = args[i + 1]
+ i += 2
+ } else {
+ i++
+ }
+ }
+ if (!bodyFile) {
+ System.err.println('error: comment requires --body-file <path>')
+ System.exit(2)
+ }
+ def f = new File(bodyFile)
+ if (!f.exists()) {
+ System.err.println("error: body file not found: ${bodyFile}")
+ System.exit(2)
+ }
+ def body = f.text
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}/comment"
+ def payload = JsonOutput.toJson([body: body])
+ def result = httpWrite(url, 'POST', payload)
+ emit([ok: true, key: key, commentId: result.id])
+}
+
+def cmd_transition(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ def transitionName = args.size() > 1 ? args[1] : null
+ if (!transitionName) {
+ System.err.println('error: transition requires a transition name')
+ System.exit(2)
+ }
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}/transitions"
+ def available = httpGet(url)
+ def match = available.transitions?.find {
+ it.name.equalsIgnoreCase(transitionName)
+ }
+ if (!match) {
+ def names = available.transitions?.collect { it.name } ?: []
+ System.err.println("error: transition '${transitionName}' not found
for ${key}; available: ${names}")
+ System.exit(3)
+ }
+ def payload = JsonOutput.toJson([transition: [id: match.id]])
+ httpWrite(url, 'POST', payload)
+ emit([ok: true, key: key, transition: match.name, transitionId: match.id])
+}
+
+def cmd_label(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ List addLabels = []
+ List removeLabels = []
+ def i = 1
+ while (i < args.size()) {
+ if (args[i] == '--add' && i + 1 < args.size()) {
+ addLabels << args[i + 1]
+ i += 2
+ } else if (args[i] == '--remove' && i + 1 < args.size()) {
+ removeLabels << args[i + 1]
+ i += 2
+ } else {
+ i++
+ }
+ }
+ if (!addLabels && !removeLabels) {
+ System.err.println('error: label requires at least one --add or
--remove flag')
+ System.exit(2)
+ }
+ def issueUrl = "${TRACKER_URL}/rest/api/2/issue/${key}?fields=labels"
+ def current = httpGet(issueUrl)
+ def labels = (current.fields?.labels ?: []) as List
+ labels.addAll(addLabels)
+ labels.removeAll(removeLabels)
+ labels = labels.unique()
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}"
+ def payload = JsonOutput.toJson([fields: [labels: labels]])
+ httpWrite(url, 'PUT', payload)
+ emit([ok: true, key: key, labels: labels])
+}
+
+def cmd_assign(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ def username = args.size() > 1 ? args[1] : null
+ if (!username) {
+ System.err.println('error: assign requires a username')
+ System.exit(2)
+ }
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}/assignee"
+ def payload = JsonOutput.toJson([name: username])
+ httpWrite(url, 'PUT', payload)
+ emit([ok: true, key: key, assignee: username])
+}
+
+def cmd_field(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ def fieldName = args.size() > 1 ? args[1] : null
+ if (!fieldName) {
+ System.err.println('error: field requires a field name')
+ System.exit(2)
+ }
+ String value = null
+ def i = 2
+ while (i < args.size()) {
+ if (args[i] == '--value' && i + 1 < args.size()) {
+ value = args[i + 1]
+ i += 2
+ } else {
+ i++
+ }
+ }
+ if (value == null) {
+ System.err.println('error: field requires --value <value>')
+ System.exit(2)
+ }
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}"
+ def payload = JsonOutput.toJson([fields: [(fieldName): value]])
Review Comment:
imho the biggest gotcha here — `value` is always a string, so anything that
isn't a free-text/number customfield will 400 against real JIRA: priority →
`{"name": "High"}`, fixVersions → `[{"name": "1.2.3"}]`, single-select →
`{"value": "x"}`, user picker → `{"name": "jdoe"}` (DC) / `{"accountId":
"..."}` (Cloud).
Mock doesn't catch it because it accepts any payload.
wdyt about adding `--value-json '<json>'` as an alt to `--value <str>`?
Keeps the shorthand for trivial cases.
##########
tools/jira/README.md:
##########
@@ -108,31 +217,64 @@ The bridge reads its configuration from the environment:
|---|---|
| `ISSUE_TRACKER_URL` | required; e.g. `https://issues.apache.org/jira` |
| `ISSUE_TRACKER_PROJECT` | project key (e.g. `FOO`) |
+| `JIRA_API_TOKEN` | required for write subcommands; base64-encoded
`email:token` |
Review Comment:
nit: this is Atlassian Cloud phrasing, but `cmd_assign` below uses `{name:
username}` which is DC-only (Cloud needs `accountId`). Since #301 explicitly
targets `issues.apache.org/jira` (DC), the impl is right — just the doc reads
Cloud-ish.
Maybe:
> base64 of `username:password` or `username:<PAT>` for Apache JIRA (Data
Center). Cloud not currently supported (`assign` uses DC `name`, not Cloud
`accountId`).
##########
tools/jira/bridge.groovy:
##########
@@ -55,6 +64,76 @@ def httpGet(String urlStr) {
return new JsonSlurper().parse(conn.inputStream)
}
+def httpWrite(String urlStr, String method, String jsonBody) {
+ def url = new URL(urlStr)
+ def conn = url.openConnection()
+ conn.requestMethod = method
+ conn.doOutput = true
+ conn.setRequestProperty('Content-Type', 'application/json')
+ conn.setRequestProperty('Accept', 'application/json')
+ if (!API_TOKEN) {
+ System.err.println('error: JIRA_API_TOKEN is required for write
operations')
+ System.exit(2)
+ }
+ conn.setRequestProperty('Authorization', "Basic ${API_TOKEN}")
Review Comment:
heads-up: ASF JIRA DC PATs are sent as `Authorization: Bearer <pat>`, not
Basic. Today a PAT user has to wrap it as `base64(user:pat)` to fake Basic,
which bypasses the canonical PAT path.
imho worth a `JIRA_AUTH_SCHEME` env (default `Basic`, accepts `Bearer`) —
ASF has been nudging projects toward PAT-only.
##########
tools/jira/bridge.groovy:
##########
@@ -130,20 +202,172 @@ def cmd_projects(List args) {
])
}
+def cmd_comment(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ String bodyFile = null
+ def i = 1
+ while (i < args.size()) {
+ if (args[i] == '--body-file' && i + 1 < args.size()) {
+ bodyFile = args[i + 1]
+ i += 2
+ } else {
+ i++
+ }
+ }
+ if (!bodyFile) {
+ System.err.println('error: comment requires --body-file <path>')
+ System.exit(2)
+ }
+ def f = new File(bodyFile)
+ if (!f.exists()) {
+ System.err.println("error: body file not found: ${bodyFile}")
+ System.exit(2)
+ }
+ def body = f.text
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}/comment"
+ def payload = JsonOutput.toJson([body: body])
+ def result = httpWrite(url, 'POST', payload)
+ emit([ok: true, key: key, commentId: result.id])
+}
+
+def cmd_transition(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ def transitionName = args.size() > 1 ? args[1] : null
+ if (!transitionName) {
+ System.err.println('error: transition requires a transition name')
+ System.exit(2)
+ }
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}/transitions"
+ def available = httpGet(url)
+ def match = available.transitions?.find {
+ it.name.equalsIgnoreCase(transitionName)
+ }
+ if (!match) {
+ def names = available.transitions?.collect { it.name } ?: []
+ System.err.println("error: transition '${transitionName}' not found
for ${key}; available: ${names}")
+ System.exit(3)
+ }
+ def payload = JsonOutput.toJson([transition: [id: match.id]])
+ httpWrite(url, 'POST', payload)
+ emit([ok: true, key: key, transition: match.name, transitionId: match.id])
+}
+
+def cmd_label(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ List addLabels = []
+ List removeLabels = []
+ def i = 1
+ while (i < args.size()) {
+ if (args[i] == '--add' && i + 1 < args.size()) {
+ addLabels << args[i + 1]
+ i += 2
+ } else if (args[i] == '--remove' && i + 1 < args.size()) {
+ removeLabels << args[i + 1]
+ i += 2
+ } else {
+ i++
+ }
+ }
+ if (!addLabels && !removeLabels) {
+ System.err.println('error: label requires at least one --add or
--remove flag')
+ System.exit(2)
+ }
+ def issueUrl = "${TRACKER_URL}/rest/api/2/issue/${key}?fields=labels"
+ def current = httpGet(issueUrl)
+ def labels = (current.fields?.labels ?: []) as List
+ labels.addAll(addLabels)
+ labels.removeAll(removeLabels)
+ labels = labels.unique()
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}"
+ def payload = JsonOutput.toJson([fields: [labels: labels]])
Review Comment:
nit: read-modify-write loses concurrent label updates. JIRA REST does this
atomically in one PUT:
```json
{"update": {"labels": [{"add": "x"}, {"remove": "y"}]}}
```
One round-trip, no race, server handles the merge. Not blocking — fine as a
follow-up.
##########
tools/jira/bridge.groovy:
##########
@@ -130,20 +202,172 @@ def cmd_projects(List args) {
])
}
+def cmd_comment(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ String bodyFile = null
+ def i = 1
+ while (i < args.size()) {
+ if (args[i] == '--body-file' && i + 1 < args.size()) {
+ bodyFile = args[i + 1]
+ i += 2
+ } else {
+ i++
+ }
+ }
+ if (!bodyFile) {
+ System.err.println('error: comment requires --body-file <path>')
+ System.exit(2)
+ }
+ def f = new File(bodyFile)
+ if (!f.exists()) {
+ System.err.println("error: body file not found: ${bodyFile}")
+ System.exit(2)
+ }
+ def body = f.text
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}/comment"
+ def payload = JsonOutput.toJson([body: body])
+ def result = httpWrite(url, 'POST', payload)
+ emit([ok: true, key: key, commentId: result.id])
+}
+
+def cmd_transition(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ def transitionName = args.size() > 1 ? args[1] : null
+ if (!transitionName) {
+ System.err.println('error: transition requires a transition name')
+ System.exit(2)
+ }
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}/transitions"
+ def available = httpGet(url)
+ def match = available.transitions?.find {
+ it.name.equalsIgnoreCase(transitionName)
+ }
+ if (!match) {
+ def names = available.transitions?.collect { it.name } ?: []
+ System.err.println("error: transition '${transitionName}' not found
for ${key}; available: ${names}")
+ System.exit(3)
+ }
+ def payload = JsonOutput.toJson([transition: [id: match.id]])
+ httpWrite(url, 'POST', payload)
+ emit([ok: true, key: key, transition: match.name, transitionId: match.id])
+}
+
+def cmd_label(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ List addLabels = []
+ List removeLabels = []
+ def i = 1
+ while (i < args.size()) {
+ if (args[i] == '--add' && i + 1 < args.size()) {
+ addLabels << args[i + 1]
+ i += 2
+ } else if (args[i] == '--remove' && i + 1 < args.size()) {
+ removeLabels << args[i + 1]
+ i += 2
+ } else {
+ i++
+ }
+ }
+ if (!addLabels && !removeLabels) {
+ System.err.println('error: label requires at least one --add or
--remove flag')
+ System.exit(2)
+ }
+ def issueUrl = "${TRACKER_URL}/rest/api/2/issue/${key}?fields=labels"
+ def current = httpGet(issueUrl)
+ def labels = (current.fields?.labels ?: []) as List
+ labels.addAll(addLabels)
+ labels.removeAll(removeLabels)
+ labels = labels.unique()
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}"
+ def payload = JsonOutput.toJson([fields: [labels: labels]])
+ httpWrite(url, 'PUT', payload)
+ emit([ok: true, key: key, labels: labels])
+}
+
+def cmd_assign(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ def username = args.size() > 1 ? args[1] : null
+ if (!username) {
+ System.err.println('error: assign requires a username')
+ System.exit(2)
+ }
+ def url = "${TRACKER_URL}/rest/api/2/issue/${key}/assignee"
+ def payload = JsonOutput.toJson([name: username])
Review Comment:
fyi: `{name: username}` is DC-only. Cloud needs `{accountId: "..."}`. Fine
for #301's scope (ASF JIRA = DC) — just calling it out so the limitation is
explicit and we don't silently break Cloud adopters later.
Maybe drop a `// DC payload — Cloud uses accountId` next to it.
##########
tools/jira/tests/test_bridge_write.py:
##########
@@ -0,0 +1,395 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for bridge.groovy write subcommands.
+
+Strategy: spin up a lightweight HTTP mock server that records requests
+and returns canned JIRA API responses, then invoke bridge.groovy via
+subprocess against it. Requires ``groovy`` on PATH.
+"""
+
+from __future__ import annotations
+
+import json
+import os
+import shutil
+import subprocess
+import threading
+from http.server import BaseHTTPRequestHandler, HTTPServer
+from pathlib import Path
+from typing import Any
+
+import pytest
+
+BRIDGE = Path(__file__).parent.parent / "bridge.groovy"
+GROOVY = shutil.which("groovy")
+
+pytestmark = pytest.mark.skipif(
+ GROOVY is None, reason="groovy not found on PATH"
+)
+
+
+# ---------------------------------------------------------------------------
+# Mock JIRA server
+# ---------------------------------------------------------------------------
+
+class _RequestLog:
+ def __init__(self) -> None:
+ self.requests: list[dict[str, Any]] = []
+
+ def clear(self) -> None:
+ self.requests.clear()
+
+
+_log = _RequestLog()
+_canned_responses: dict[str, tuple[int, Any]] = {}
+
+
+class _JiraHandler(BaseHTTPRequestHandler):
+ def _read_body(self) -> bytes:
+ length = int(self.headers.get("Content-Length", 0))
+ return self.rfile.read(length) if length else b""
+
+ def _handle(self, method: str) -> None:
+ body = self._read_body()
+ entry: dict[str, Any] = {
+ "method": method,
+ "path": self.path,
+ "headers": dict(self.headers),
+ }
+ if body:
+ content_type = self.headers.get("Content-Type", "")
+ if "application/json" in content_type:
+ entry["body"] = json.loads(body)
+ else:
+ entry["body_raw"] = body
+ _log.requests.append(entry)
+
+ key = f"{method} {self.path.split('?')[0]}"
+ if key in _canned_responses:
+ status, payload = _canned_responses[key]
+ else:
+ status, payload = 200, {}
Review Comment:
nit on coverage: mock defaults to `200 + {}` for unmapped routes, so any
malformed body slips through silently. A "strict" mode that 400s on unknown
keys / wrong shapes would catch the `cmd_field` shape issue I flagged in the
groovy file.
Also — `Authorization` header value isn't asserted anywhere (only
presence/absence). Worth a sanity check that the scheme + token round-trip
correctly.
##########
tools/jira/bridge.groovy:
##########
@@ -130,20 +202,172 @@ def cmd_projects(List args) {
])
}
+def cmd_comment(List args) {
+ def key = args ? args[0] : null
+ validateKey(key)
+ String bodyFile = null
+ def i = 1
+ while (i < args.size()) {
+ if (args[i] == '--body-file' && i + 1 < args.size()) {
+ bodyFile = args[i + 1]
+ i += 2
+ } else {
+ i++
+ }
+ }
+ if (!bodyFile) {
+ System.err.println('error: comment requires --body-file <path>')
+ System.exit(2)
+ }
+ def f = new File(bodyFile)
+ if (!f.exists()) {
+ System.err.println("error: body file not found: ${bodyFile}")
+ System.exit(2)
+ }
+ def body = f.text
Review Comment:
nit: `f.text` uses platform default encoding. Non-ASCII comment bodies can
corrupt on non-UTF-8 hosts. `f.getText("UTF-8")` fixes it.
##########
tools/jira/bridge.groovy:
##########
@@ -55,6 +64,76 @@ def httpGet(String urlStr) {
return new JsonSlurper().parse(conn.inputStream)
}
+def httpWrite(String urlStr, String method, String jsonBody) {
+ def url = new URL(urlStr)
+ def conn = url.openConnection()
+ conn.requestMethod = method
+ conn.doOutput = true
+ conn.setRequestProperty('Content-Type', 'application/json')
+ conn.setRequestProperty('Accept', 'application/json')
+ if (!API_TOKEN) {
+ System.err.println('error: JIRA_API_TOKEN is required for write
operations')
+ System.exit(2)
+ }
+ conn.setRequestProperty('Authorization', "Basic ${API_TOKEN}")
+ conn.connectTimeout = 10000
+ conn.readTimeout = 30000
+ conn.outputStream.withWriter('UTF-8') { it.write(jsonBody) }
+ def code = conn.responseCode
+ if (code < 200 || code >= 300) {
+ def err = conn.errorStream ? conn.errorStream.text :
conn.responseMessage
+ System.err.println("error: HTTP ${code} ${method} ${urlStr}\n${err}")
+ System.exit(3)
+ }
+ if (code == 204) return [:]
+ def text = conn.inputStream?.text
+ return text ? new JsonSlurper().parseText(text) : [:]
+}
+
+def httpMultipart(String urlStr, File file) {
+ def boundary = "----BridgeBoundary${System.currentTimeMillis()}"
+ def url = new URL(urlStr)
+ def conn = url.openConnection()
+ conn.requestMethod = 'POST'
+ conn.doOutput = true
+ conn.setRequestProperty('Content-Type', "multipart/form-data;
boundary=${boundary}")
+ conn.setRequestProperty('Accept', 'application/json')
+ conn.setRequestProperty('X-Atlassian-Token', 'no-check')
+ if (!API_TOKEN) {
+ System.err.println('error: JIRA_API_TOKEN is required for write
operations')
+ System.exit(2)
+ }
+ conn.setRequestProperty('Authorization', "Basic ${API_TOKEN}")
+ conn.connectTimeout = 10000
+ conn.readTimeout = 60000
+ def os = conn.outputStream
+ os.write("--${boundary}\r\n".getBytes('UTF-8'))
+ os.write("Content-Disposition: form-data; name=\"file\";
filename=\"${file.name}\"\r\n".getBytes('UTF-8'))
+ os.write("Content-Type:
application/octet-stream\r\n\r\n".getBytes('UTF-8'))
+ file.withInputStream { is -> os << is }
Review Comment:
nit: no size guard on the upload, and filename in `Content-Disposition`
isn't escaped. Source is local FS so risk is low, but a soft cap (e.g. 10MB)
would prevent foot-guns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
