-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat(cli): option to init via username password, add lint for smoke-test #9675
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
d7dd717
feat(cli): option to get token via username pass in init
anshbansal e33b9b0
code review feedback
anshbansal a2cc185
missed method name change
anshbansal 3be4e00
extract existing code to avoid circular dependencies
anshbansal a950793
wip add lint smoke-test
anshbansal 5c55d43
fix some lint issues
anshbansal 931553f
fix rebase conflicts
anshbansal f69f5f3
fix test path
anshbansal 9f3fe68
fix lint rules
hsheth2 074889a
add lint in ci for smoke test
anshbansal 91f87ba
more fixes
anshbansal f141534
try to fix CI lint not starting
anshbansal 0ba1c3a
rename file from - to _
anshbansal 425b79a
more fixes
anshbansal 1e32526
more lint fix
anshbansal 3325279
fix more lint
anshbansal 52da483
fix CI
anshbansal 75adfc9
fix CI after rebase
anshbansal 427f123
remove unused method
anshbansal eb09b65
guess entity type, revert test changes
anshbansal 0834a74
temporary skip tests where functionality needs to be validated
anshbansal 7ba0f96
changes for improvements in patch proposal
anshbansal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
import logging | ||
import os | ||
import sys | ||
from typing import Optional, Union | ||
|
||
import click | ||
import yaml | ||
from pydantic import BaseModel, ValidationError | ||
|
||
from datahub.cli.env_utils import get_boolean_env_variable | ||
|
||
__help__ = ( | ||
"For helper methods to contain manipulation of the config file in local system." | ||
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. module level docstrings should go at the top and be in 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. Going to do this in follow up PR. |
||
) | ||
log = logging.getLogger(__name__) | ||
|
||
DEFAULT_GMS_HOST = "http://localhost:8080" | ||
CONDENSED_DATAHUB_CONFIG_PATH = "~/.datahubenv" | ||
DATAHUB_CONFIG_PATH = os.path.expanduser(CONDENSED_DATAHUB_CONFIG_PATH) | ||
DATAHUB_ROOT_FOLDER = os.path.expanduser("~/.datahub") | ||
ENV_SKIP_CONFIG = "DATAHUB_SKIP_CONFIG" | ||
|
||
|
||
class GmsConfig(BaseModel): | ||
server: str | ||
token: Optional[str] = None | ||
|
||
|
||
class DatahubConfig(BaseModel): | ||
gms: GmsConfig | ||
|
||
|
||
def persist_datahub_config(config: dict) -> None: | ||
with open(DATAHUB_CONFIG_PATH, "w+") as outfile: | ||
yaml.dump(config, outfile, default_flow_style=False) | ||
return None | ||
|
||
|
||
def write_gms_config( | ||
host: str, token: Optional[str], merge_with_previous: bool = True | ||
) -> None: | ||
config = DatahubConfig(gms=GmsConfig(server=host, token=token)) | ||
if merge_with_previous: | ||
try: | ||
previous_config = get_client_config(as_dict=True) | ||
assert isinstance(previous_config, dict) | ||
except Exception as e: | ||
# ok to fail on this | ||
previous_config = {} | ||
log.debug( | ||
f"Failed to retrieve config from file {DATAHUB_CONFIG_PATH}: {e}. This isn't fatal." | ||
) | ||
config_dict = {**previous_config, **config.dict()} | ||
else: | ||
config_dict = config.dict() | ||
persist_datahub_config(config_dict) | ||
|
||
|
||
def get_details_from_config(): | ||
datahub_config = get_client_config(as_dict=False) | ||
assert isinstance(datahub_config, DatahubConfig) | ||
if datahub_config is not None: | ||
gms_config = datahub_config.gms | ||
|
||
gms_host = gms_config.server | ||
gms_token = gms_config.token | ||
return gms_host, gms_token | ||
else: | ||
return None, None | ||
|
||
|
||
def should_skip_config() -> bool: | ||
return get_boolean_env_variable(ENV_SKIP_CONFIG, False) | ||
|
||
|
||
def ensure_datahub_config() -> None: | ||
if not os.path.isfile(DATAHUB_CONFIG_PATH): | ||
click.secho( | ||
f"No {CONDENSED_DATAHUB_CONFIG_PATH} file found, generating one for you...", | ||
bold=True, | ||
) | ||
write_gms_config(DEFAULT_GMS_HOST, None) | ||
|
||
|
||
def get_client_config(as_dict: bool = False) -> Union[Optional[DatahubConfig], dict]: | ||
with open(DATAHUB_CONFIG_PATH, "r") as stream: | ||
try: | ||
config_json = yaml.safe_load(stream) | ||
if as_dict: | ||
return config_json | ||
try: | ||
datahub_config = DatahubConfig.parse_obj(config_json) | ||
return datahub_config | ||
except ValidationError as e: | ||
click.echo( | ||
f"Received error, please check your {CONDENSED_DATAHUB_CONFIG_PATH}" | ||
) | ||
click.echo(e, err=True) | ||
sys.exit(1) | ||
except yaml.YAMLError as exc: | ||
click.secho(f"{DATAHUB_CONFIG_PATH} malformed, error: {exc}", bold=True) | ||
return None |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
this should not be in the setup job, since it'll block the rest of the docker builds
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.
That is actually the intention. Lint should cause rest of CI to simply not run. This causes anyone making changes in smoke tests to fail fast. Otherwise CI will wait for docker builds which takes a while before failing.
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 not sure I agree with that - this will add a few minutes of wall clock time to every docker-unified run, which is already the slowest part of our CI. Lint issues are going to be relatively rare in smoke test, so I don't think it makes sense to slow down our CI significantly to avoid a small number of wasted docker builds
Instead the lint should be a separate job so that the docker builds can run in parallel, and we can use something like this https://github.com/orgs/community/discussions/38361 to cancel the other workflows