JIRA cloud issue creation and availability issue#448
JIRA cloud issue creation and availability issue#448ralphbean merged 1 commit intorelease-engineering:mainfrom
Conversation
There was a problem hiding this comment.
I'm OK with this as is, since we very much want to stop creating duplicates, but I have a couple of concerns (and a bunch of suggestions).
First, I recommend changing the focus of _get_existing_jira_issue_query() (and its name) so that it returns the key list and not the JQL -- the caller can produce the JQL easily enough, and returning a naked tuple is something best to avoid unless there's a compelling need.
Second, you modified two access to use displayName, but their respective approaches are inconsistent...which is fine if it's justified, but my instinct is that one of them is "right" and one is "wrong" -- and I don't know which is which....
So, perhaps we could merge this and then you could polish it in a follow-up PR.
| issue_keys, jql = _get_existing_jira_issue_query(issue) | ||
| if not jql: | ||
| return None | ||
| results: ResultList[JIssue] = client.search_issues(jql) |
There was a problem hiding this comment.
I don't think it's worthwhile to return a tuple. Instead, I suggest refocusing (and renaming) the subroutine and moving the query creation to the caller:
| issue_keys, jql = _get_existing_jira_issue_query(issue) | |
| if not jql: | |
| return None | |
| results: ResultList[JIssue] = client.search_issues(jql) | |
| issue_keys = _get_existing_jira_issue_keys(issue) | |
| if not issue_keys: | |
| return None | |
| jql = f"key in ({','.join(issue_keys)}) | |
| results: ResultList[JIssue] = client.search_issues(jql") |
| if not results: | ||
| # It is _possible_ that the downstream issue exists (i.e., we did hit | ||
| # it in the cache or in Snowflake) but the Jira search cannot find it. | ||
| # (This happens when the issue has been archived.) Fail as gracefully | ||
| # as we can here, but our caller will probably create it again. | ||
| log.warning( | ||
| "Previously-existing downstream issue %s not found for upstream issue %s.", | ||
| issue.id, | ||
| issue.url, | ||
| ) | ||
| return None | ||
| # JQL/search index can lag right after create, or hide archived issues. | ||
| # Resolve by issue key (direct GET) before giving up and duplicating. | ||
| results = ResultList[JIssue]() |
There was a problem hiding this comment.
You don't need to assign results here -- the if statement at line 323 ensures that it's an empty list already.
| for key in issue_keys: | ||
| try: | ||
| found = client.issue(key) | ||
| except JIRAError: | ||
| continue | ||
| log.info( | ||
| "JQL missed key %s for upstream %s; using direct issue fetch.", | ||
| key, | ||
| issue.url, | ||
| ) | ||
| results = ResultList[JIssue]((found,)) | ||
| break | ||
| if not results: |
There was a problem hiding this comment.
Rather than using if not results, I think it would be more graceful to just use else -- if we don't break out of the for loop, then we'll execute the else block.
| author_label = getattr(author, "displayName", None) or getattr( | ||
| author, "name", None | ||
| ) |
There was a problem hiding this comment.
Alternatively,
| author_label = getattr(author, "displayName", None) or getattr( | |
| author, "name", None | |
| ) | |
| author_label = getattr(author, "displayName", getattr(author, "name", None)) |
| update = ( | ||
| issue.downstream.get("owner") != existing.fields.assignee.displayName | ||
| ) |
There was a problem hiding this comment.
Why do we use getattr() to access displayName at line 463 but use a direct access here? (Is one of the two "wrong"?)
|
|
||
| def _q(keys): |
There was a problem hiding this comment.
Access to this function is scoped to this function, so you don't need the underscore prefix on the name.
| if x["issue_side_effect"] is not None: | ||
| mock_client.issue.side_effect = x["issue_side_effect"] | ||
| else: | ||
| mock_client.issue.side_effect = None | ||
| mock_client.issue.return_value = mock_issue_1 |
There was a problem hiding this comment.
I believe that you can simplify this:
| if x["issue_side_effect"] is not None: | |
| mock_client.issue.side_effect = x["issue_side_effect"] | |
| else: | |
| mock_client.issue.side_effect = None | |
| mock_client.issue.return_value = mock_issue_1 | |
| mock_client.issue.side_effect = x["issue_side_effect"] | |
| if not x["issue_side_effect"]: | |
| mock_client.issue.return_value = mock_issue_1 |
or possibly even
| if x["issue_side_effect"] is not None: | |
| mock_client.issue.side_effect = x["issue_side_effect"] | |
| else: | |
| mock_client.issue.side_effect = None | |
| mock_client.issue.return_value = mock_issue_1 | |
| if not mock_client.issue.side_effect := x["issue_side_effect"]: | |
| mock_client.issue.return_value = mock_issue_1 |
3 participants
Created this PR for solving the following issue:
After jira cloud issue creation instantly if you search for through JQL it won't return so for that we have used GET call as an alternative if the key is present in cache or in snowflake but not in JQL ...
As previously we are logging issue Id but it isn't that useful so we are now chnaged to the log the JQL contents it will help to debug in better way.
Updated to use displayName instead of name in
_update_Assigneeandcheck_comments_for_duplicate