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 16369 revert back api access change #90

Merged
merged 7 commits into from
Nov 19, 2021

Conversation

prijendev
Copy link
Contributor

Description of change

  • Revert backed API access change in discover mode.
  • Now, if at least one stream have read permission then tap just print warning message for all streams which does not have read permission
  • If any one of stream does not have read permission then raise error.

Manual QA steps

  • Create access token with limited scope and verify tap behaviour.

Risks

Rollback steps

  • revert this branch

# which does not have read permission
LOGGER.warning(message)
else:
# If any one of stream does not have read permission then raise error.
Copy link
Contributor

Choose a reason for hiding this comment

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

@prijendev The comment here is wrong.
This code snippet depicts that. "If none of the streams are having the 'read' access, then the code will raise an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment

"The account credentials supplied do not have read access for the following stream(s): {}".format(streams_name)
raise ZendeskForbiddenError(message)
message = "The account credentials supplied do not have read access for the following stream(s): {}."\
"The data for the mentioned streams will not be collected if selected due to a lack of 'read' permission.".format(streams_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

@prijendev Update the message to,
"'The account credentials supplied do not have 'read' access to the following stream(s): {}. The data for these streams would not be collected due to lack of required permission"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated message

@@ -58,10 +58,18 @@ def discover_streams(client, config):
streams.append({'stream': stream.name, 'tap_stream_id': stream.name, 'schema': schema, 'metadata': stream.load_metadata()})
Copy link
Contributor

Choose a reason for hiding this comment

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

@prijendev If the stream would not have 'read' access, would we enter the JSON for that stream in the catalog?

Copy link
Contributor Author

@prijendev prijendev Nov 16, 2021

Choose a reason for hiding this comment

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

Yes, we would enter the JSON for that stream in catalog

# which does not have read permission
LOGGER.warning(message)
else:
message = "HTTP-error-code: 403, Error: You are missing the following required scopes: read. "\
"The account credentials supplied do not have read access for the following stream(s): {}".format(streams_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

@prijendev Can you also update this message as,
"HTTP-error-code: 403, Error: The account credentials supplied do not have 'read' access to any of streams supported by the tap. Data collection cannot be initiated due to lack of permissions."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@KrisPersonal KrisPersonal merged commit 4f015ba into master Nov 19, 2021
@KrisPersonal KrisPersonal deleted the TDL-16369-Revert-Back-API-Access-Change branch November 19, 2021 23:06
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.

4 participants