Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

896 - GDPR Update and CI fixes #909

Closed
wants to merge 5 commits into from
Closed

896 - GDPR Update and CI fixes #909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f0e45d
896 - Require auth creds from environment
csullivannet Mar 2, 2020
fd487fe
896 - Fix removed field 'key'
csullivannet Feb 28, 2020
7631559
896 - Replace instances of names with accountIds
csullivannet Feb 28, 2020
326f84e
896 - Add mandatory permissions parameter
csullivannet Mar 2, 2020
9743e54
896 - Linter fixes
csullivannet Mar 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 22 additions & 14 deletions jira/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,9 +481,8 @@ def __init__(
self._create_kerberos_session(timeout, kerberos_options=kerberos_options)
elif auth:
self._create_cookie_auth(auth, timeout)
validate = (
True
) # always log in for cookie based auth, as we need a first request to be logged in
# always log in for cookie based auth, as we need a first request to be logged in
validate = True
else:
verify = self._options["verify"]
self._session = ResilientSession(timeout=timeout)
Expand Down Expand Up @@ -944,7 +943,7 @@ def create_component(
name,
project,
description=None,
leadUserName=None,
leadAccountId=None,
assigneeType=None,
isAssigneeTypeValid=False,
):
Expand All @@ -956,8 +955,8 @@ def create_component(
:type project: str
:param description: a description of the component
:type description: str
:param leadUserName: the username of the user responsible for this component
:type leadUserName: Optional[str]
:param leadAccountId: the username of the user responsible for this component
:type leadAccountId: Optional[str]
:param assigneeType: see the ComponentBean.AssigneeType class for valid values
:type assigneeType: Optional[str]
:param isAssigneeTypeValid: boolean specifying whether the assignee type is acceptable (Default: False)
Expand All @@ -971,8 +970,8 @@ def create_component(
}
if description is not None:
data["description"] = description
if leadUserName is not None:
data["leadUserName"] = leadUserName
if leadAccountId is not None:
data["leadAccountId"] = leadAccountId
if assigneeType is not None:
data["assigneeType"] = assigneeType

Expand Down Expand Up @@ -1569,7 +1568,7 @@ def _get_user_accountid(self, user):

# non-resource
@translate_resource_args
def assign_issue(self, issue, assignee):
def assign_issue(self, issue, assignee_id):
"""Assign an issue to a user. None will set it to unassigned. -1 will set it to Automatic.

:param issue: the issue ID or key to assign
Expand All @@ -1585,7 +1584,7 @@ def assign_issue(self, issue, assignee):
+ str(issue)
+ "/assignee"
)
payload = {"accountId": self._get_user_accountid(assignee)}
payload = {"accountId": assignee_id}
# 'key' and 'name' are deprecated in favor of accountId
r = self._session.put(url, data=json.dumps(payload))
raise_on_error(r)
Expand Down Expand Up @@ -2176,7 +2175,13 @@ def request_type_by_name(self, service_desk, name):

# non-resource
def my_permissions(
self, projectKey=None, projectId=None, issueKey=None, issueId=None
self,
projectKey=None,
projectId=None,
issueKey=None,
issueId=None,
*,
permissions
):
"""Get a dict of all available permissions on the server.

Expand All @@ -2188,6 +2193,8 @@ def my_permissions(
:type issueKey: Optional[str]
:param issueId: limit returned permissions to the specified issue
:type issueId: Optional[str]
:param permissions: a CSV list of permission keys to get permissions for
:type permissions: str
:rtype: Dict[str, Dict[str, Dict[str, str]]]
"""
params = {}
Expand All @@ -2199,6 +2206,7 @@ def my_permissions(
params["issueKey"] = issueKey
if issueId is not None:
params["issueId"] = issueId
params["permissions"] = permissions
return self._get_json("mypermissions", params=params)

# Priorities
Expand Down Expand Up @@ -3438,9 +3446,9 @@ def backup_download(self, filename=None):
logging.error(ioe)
return None

def current_user(self, field="key"):
"""Returns the username or emailAddress of the current user. For anonymous
users it will return a value that evaluates as False.
def current_user(self, field="accountId"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not backwards compatible and will break non-cloud versions of Jira.

Copy link
Collaborator

@Addono Addono Mar 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I noticed that similar BC breaking changes have already been committed to master (e.g. a4e6b1b). Not too fond of this, but I guess restoring the BC could better be left for a separate PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI tests are only against the cloud version, which seems to be the priority for this project. Given the limited resources available to maintain it, I would tend to agree with breaking older on-prem versions in favour of keeping track with the cloud and maintaining BC on a best-effort basis.

The GDPR compliance changes also represent breaking changes within a major version of the API. This represents a fair amount of work to restore functionality to the library as more things get deprecated and dropped. As far as I can tell it also has caused a fair amount of divergence between on-prem and cloud versions, increasing the difficulty of maintenance.

With these factors in mind I'd prefer to prioritize getting fixes in to bring CI back to green and raise issues that arise afterwards, so that the project can gain momentum again. With CI green, it should be easier to put forward smaller PRs that are easier to merge to fix BC piece-by-piece.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that getting the CI green again should be a top priority. Over the weekend I was working on getting the CI to run against a Jira instance spinned up as part of the pipeline, as to get rid of the credential management issue and have more reproducible builds. So in that effort fixing CI and maintaining BC align.

Anyway, this was just the first BC breaking change I ran into. Shouldn't be considered a blocker for this PR.

"""Returns information about the current user, including but not limited
to their display name, e-mail, accountId.

:rtype: str
"""
Expand Down
6 changes: 3 additions & 3 deletions jira/resources.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,17 +843,17 @@ class User(Resource):
"""A JIRA user."""

def __init__(self, options, session, raw=None):
Resource.__init__(self, "user?username={0}", options, session)
Resource.__init__(self, "user?accountId={0}", options, session)
if raw:
self._parse_raw(raw)

def __hash__(self):
"""Hash calculation."""
return hash(str(self.name))
return hash(str(self.accountId))

def __eq__(self, other):
"""Comparison."""
return str(self.name) == str(other.name)
return str(self.accountId) == str(other.accountId)


class Group(Resource):
Expand Down
Loading