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.

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

TDL-19198: Provide description of groups in README #88

Merged

Conversation

jtilala
Copy link
Contributor

@jtilala jtilala commented May 24, 2022

Description of change

  • Add description and sample values for 'groups' in the README file.

Manual QA steps

Risks

Rollback steps

  • revert this branch

@@ -67,6 +69,8 @@ This tap:
can be found. For example, it might look like:
`https://mycompany.atlassian.net`.

The `groups` specifies groups for users stream. `groups` is optional parameter. Default value of groups is ["jira-administrators", "jira-software-users", "jira-core-users", "jira-users", "site-admin", "users"].

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the default value is as per below.

groups = ["jira-administrators",
"jira-software-users",
"jira-core-users",
"jira-users",
"users"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I have updated the groups default values as shown in streams.py

README.md Outdated
@@ -41,7 +41,8 @@ This tap:
"password": "your-jira-password",
"base_url": "https://your-jira-domain",
"user_agent": "<user-agent>",
"request_timeout": 300
"request_timeout": 300,
"groups": "jira-administrators,site-admins,jira-software-users"

Choose a reason for hiding this comment

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

Suggested change
"groups": "jira-administrators,site-admins,jira-software-users"
"groups": "jira-administrators, site-admins, jira-software-users"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As shown in the code, when we split the 'groups' value, we are not removing extra spaces. So if we are giving 'groups' value with space as shown in the above suggestion, we are getting a list ["jira-administrators", " site-admins", " jira-software-users"] instead of ["jira-administrators", "site-admins", "jira-software-users"].

Choose a reason for hiding this comment

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

I think extra space should not fail the tap. Ideally after a split we should strip() the leading spaces.

Copy link
Contributor Author

@jtilala jtilala May 26, 2022

Choose a reason for hiding this comment

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

In the existing code, we are not using strip, but this suggestion is right, we should ignore extra spaces, so added strip() and also updated README as per code change.

README.md Outdated
@@ -56,7 +57,8 @@ This tap:
"cloud_id": "<cloud-id>",
"refresh_token": "<refresh-token>",
"start_date": "<i.e. 2017-12-04T19:19:32Z>",
"request_timeout": 300
"request_timeout": 300,
"groups": "jira-administrators,site-admins,jira-software-users"

Choose a reason for hiding this comment

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

Suggested change
"groups": "jira-administrators,site-admins,jira-software-users"
"groups": "jira-administrators, site-admins, jira-software-users"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed above we can not pass 'groups' value with extra spaces.

README.md Outdated
@@ -67,6 +69,8 @@ This tap:
can be found. For example, it might look like:
`https://mycompany.atlassian.net`.

The `groups` specifies groups for users stream. `groups` is optional parameter. Default value of groups is `["jira-administrators", "jira-software-users", "jira-core-users", "jira-users", "users"]`.

Choose a reason for hiding this comment

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

Suggested change
The `groups` specifies groups for users stream. `groups` is optional parameter. Default value of groups is `["jira-administrators", "jira-software-users", "jira-core-users", "jira-users", "users"]`.
The `groups` specifies groups for users stream. It is an optional parameter. Default value is `["jira-administrators", "jira-software-users", "jira-core-users", "jira-users", "users"]`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it looks good, I have updated the comment as per suggestion.

@RushiT0122 RushiT0122 self-requested a review May 26, 2022 10:43
@KrisPersonal KrisPersonal merged commit 37df463 into master Jun 1, 2022
@KrisPersonal KrisPersonal mentioned this pull request Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants