- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2
In 908 build dev testing framework #107
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
Changes from all commits
ddc79eb
              fc160e8
              1d4dab3
              fd92724
              786bdc0
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| import logging | ||
| import os | ||
| from typing import IO | ||
|  | ||
| import click | ||
|  | ||
| from carbon.app import DatabaseToFtpPipe | ||
| from carbon.app import DatabaseToFilePipe, DatabaseToFtpPipe | ||
| from carbon.config import configure_logger, configure_sentry, load_config_values | ||
| from carbon.database import DatabaseEngine | ||
| from carbon.helpers import sns_log | ||
|  | @@ -13,8 +14,32 @@ | |
|  | ||
| @click.command() | ||
| @click.version_option() | ||
| @click.option("--run_connection_tests", is_flag=True) | ||
| def main(*, run_connection_tests: bool) -> None: | ||
| @click.option( | ||
| "-o", | ||
| "--output_file", | ||
| help=( | ||
| "Name of file (including the extension) into which Carbon writes the output. " | ||
| "Defaults to None, which will write the output to an XML file on the " | ||
| "Symplectic Elements FTP server." | ||
| ), | ||
| type=click.File("wb"), | ||
| default=None, | ||
| ) | ||
| @click.option( | ||
| "--run_connection_tests", | ||
| help="Test connection to the Data Warehouse and the Symplectic Elements FTP server", | ||
| is_flag=True, | ||
| ) | ||
| @click.option( | ||
| "--use_sns_logging/--ignore_sns_logging", | ||
| help=( | ||
| "Turn on SNS logging. If SNS logging is used, notification emails " | ||
| "indicating the start and result of a Carbon run will be sent to subscribers " | ||
| "for the Carbon topic. Defaults to True." | ||
| 
      Comment on lines
    
      +36
     to 
      +38
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this description. While "SNS Logging" makes me immediately think it might be cranking out lots of logging to SNS, this makes it clear it's really just emails, occassionally. | ||
| ), | ||
| default=True, | ||
| ) | ||
| def main(*, output_file: IO, run_connection_tests: bool, use_sns_logging: bool) -> None: | ||
| """Generate a data feed that uploads XML files to the Symplectic Elements FTP server. | ||
|  | ||
| The feed uses a SQLAlchemy engine to connect to the Data Warehouse. A query is | ||
|  | @@ -44,20 +69,32 @@ def main(*, run_connection_tests: bool) -> None: | |
| ) | ||
|  | ||
| engine = DatabaseEngine() | ||
| engine.configure(config_values["CONNECTION_STRING"], thick_mode=True) | ||
|  | ||
| # test connection to the Data Warehouse | ||
| engine.configure(config_values["CONNECTION_STRING"], thick_mode=True) | ||
| engine.run_connection_test() | ||
|  | ||
| # test connection to the Symplectic Elements FTP server | ||
| pipe = DatabaseToFtpPipe(config=config_values, engine=engine) | ||
| pipe.run_connection_test() | ||
| pipe: DatabaseToFtpPipe | DatabaseToFilePipe | ||
| if output_file: | ||
| pipe = DatabaseToFilePipe( | ||
| config=config_values, engine=engine, output_file=output_file | ||
| ) | ||
| else: | ||
| pipe = DatabaseToFtpPipe(config=config_values, engine=engine) | ||
| # test connection to the Symplectic Elements FTP server | ||
| pipe.run_connection_test() | ||
|  | ||
| if not run_connection_tests: | ||
| sns_log(config_values=config_values, status="start") | ||
| logger.info("Carbon run has started.") | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this refactored block | ||
| if use_sns_logging: | ||
| sns_log(config_values=config_values, status="start") | ||
| try: | ||
| pipe.run() | ||
| except Exception as error: # noqa: BLE001 | ||
| sns_log(config_values=config_values, status="fail", error=error) | ||
| logger.info("Carbon run has failed.") | ||
| if use_sns_logging: | ||
| sns_log(config_values=config_values, status="fail", error=error) | ||
| else: | ||
| sns_log(config_values=config_values, status="success") | ||
| logger.info("Carbon run has successfully completed.") | ||
| if use_sns_logging: | ||
| sns_log(config_values=config_values, status="success") | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -6,8 +6,6 @@ | |
|  | ||
| ENV_VARS = [ | ||
| "FEED_TYPE", | ||
| "LOG_LEVEL", | ||
| "SENTRY_DSN", | ||
| "SNS_TOPIC", | ||
| "SYMPLECTIC_FTP_PATH", | ||
| "WORKSPACE", | ||
|  | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghukill Your thoughts on this formatting for comments in copy/paste
.envsection? I think you did it differently fortimdex-simulatorand I think it's worth standardizing early on but I don't have strong feelings on either approach, either works for me!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this format. Personally, I'm in support of comments above env vars, as that would support multi-line comments if we ever need them. Yes, it's busy, and takes up space, but that's okay by me.
A couple minor nit picks:
WORKSPACEWORKSPACE, as it is common to most repos, and worry those comments would drift / conflict / confuseAt the end of the day, I'd really just imagine you're new to the codebase: what do you want communicated to you when you first look at this? I'd hope for really important and/or really obvious ones at the top, likely without comments. Then little sections of interelated ones, with comments as needed.
Just as an example, for this repo me knee-jerk expectation might be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this!