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

[low-code connectors] fix so we don't display yaml when debug flag is turned off #15383

Merged
merged 2 commits into from
Aug 8, 2022

Conversation

brianjlai
Copy link
Contributor

What

For some reason I put the debug statement in the wrong place. It's poor DX to get debug statements when you don't have the --debug on.

How

We store the parsed yaml on the declarative stream. We can just print it later in the streams()method and at this point the debug/info level has been properly set.

Note: We don't need to do this for check() because check invokes the streams() method. Otherwise check would print it twice

@brianjlai brianjlai requested review from girarda and sherifnada August 5, 2022 23:46
@brianjlai brianjlai requested a review from a team as a code owner August 5, 2022 23:46
@brianjlai brianjlai changed the title fix so we don't display yaml when debug is turned off [low-code connectors] fix so we don't display yaml when debug flag is turned off Aug 5, 2022
@github-actions github-actions bot added the CDK Connector Development Kit label Aug 5, 2022
@@ -42,9 +48,4 @@ def streams(self, config: Mapping[str, Any]) -> List[Stream]:
def _read_and_parse_yaml_file(self, path_to_yaml_file):
with open(path_to_yaml_file, "r") as f:
config_content = f.read()
parsed_config = YamlParser().parse(config_content)
Copy link
Contributor

Choose a reason for hiding this comment

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

how does moving this help? the debug level was already set above it no?

Copy link
Contributor Author

@brianjlai brianjlai Aug 6, 2022

Choose a reason for hiding this comment

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

Actually when we construct the declarative source in _read_and_parse_yaml_file() in source.py (here's how its implemented for source-sendgrid), this is done before we start entrypoint.py.launch(). main.py for sengrid

So we don't actually know yet if the user specified --debug since that gets interpreted in launch(). When we move it to streams()this gets called from entrypoint.py after log level is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and the above debug level should be removed since its effectively a no-op, since it gets reassigned in entrypoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add a todo to resolve this but nbd for this pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the debug level in the above code, so I don't think we'd need any follow ups. As it stands now the flow of the read command is:

  1. parse yaml
  2. save it to self._source_config
  3. parse command line args in entrypoint
  4. assign logger level to DEBUG or INFO based on presence of --debug
  5. handle read
  6. emit debug message with parsed YAML if on
  7. process sync

@brianjlai brianjlai merged commit ef712f1 into master Aug 8, 2022
@brianjlai brianjlai deleted the brian/move_parsed_yaml_debug branch August 8, 2022 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants