From f82332b8a6340bc29bb8713f19f052b994895f17 Mon Sep 17 00:00:00 2001 From: benpankow Date: Tue, 7 May 2024 16:23:17 -0700 Subject: [PATCH] respond to review comments --- python_modules/dagster/dagster/__init__.py | 4 ++ .../_core/definitions/metadata/__init__.py | 2 +- .../_core/definitions/metadata/source_code.py | 48 +++++++++++++------ .../test_asset_defs_source_metadata.py | 8 ++-- 4 files changed, 42 insertions(+), 20 deletions(-) diff --git a/python_modules/dagster/dagster/__init__.py b/python_modules/dagster/dagster/__init__.py index a92508ced6792..fff390fd13945 100644 --- a/python_modules/dagster/dagster/__init__.py +++ b/python_modules/dagster/dagster/__init__.py @@ -266,12 +266,14 @@ ) from dagster._core.definitions.metadata import ( BoolMetadataValue as BoolMetadataValue, + CodeReferencesMetadataValue as CodeReferencesMetadataValue, DagsterAssetMetadataValue as DagsterAssetMetadataValue, DagsterJobMetadataValue as DagsterJobMetadataValue, DagsterRunMetadataValue as DagsterRunMetadataValue, FloatMetadataValue as FloatMetadataValue, IntMetadataValue as IntMetadataValue, JsonMetadataValue as JsonMetadataValue, + LocalFileCodeReference as LocalFileCodeReference, MarkdownMetadataValue as MarkdownMetadataValue, MetadataEntry as MetadataEntry, MetadataValue as MetadataValue, @@ -284,7 +286,9 @@ TableSchemaMetadataValue as TableSchemaMetadataValue, TextMetadataValue as TextMetadataValue, TimestampMetadataValue as TimestampMetadataValue, + UrlCodeReference as UrlCodeReference, UrlMetadataValue as UrlMetadataValue, + link_to_source_control as link_to_source_control, with_source_code_references as with_source_code_references, ) from dagster._core.definitions.metadata.table import ( diff --git a/python_modules/dagster/dagster/_core/definitions/metadata/__init__.py b/python_modules/dagster/dagster/_core/definitions/metadata/__init__.py index 0a47711095545..6fb40d3f06090 100644 --- a/python_modules/dagster/dagster/_core/definitions/metadata/__init__.py +++ b/python_modules/dagster/dagster/_core/definitions/metadata/__init__.py @@ -62,7 +62,7 @@ CodeReferencesMetadataSet as CodeReferencesMetadataSet, CodeReferencesMetadataValue as CodeReferencesMetadataValue, LocalFileCodeReference as LocalFileCodeReference, - SourceControlCodeReference as SourceControlCodeReference, + UrlCodeReference as UrlCodeReference, link_to_source_control as link_to_source_control, with_source_code_references as with_source_code_references, ) diff --git a/python_modules/dagster/dagster/_core/definitions/metadata/source_code.py b/python_modules/dagster/dagster/_core/definitions/metadata/source_code.py index f915f6d670368..8c9e562d6e6a9 100644 --- a/python_modules/dagster/dagster/_core/definitions/metadata/source_code.py +++ b/python_modules/dagster/dagster/_core/definitions/metadata/source_code.py @@ -43,11 +43,12 @@ class LocalFileCodeReference(DagsterModel): @experimental @whitelist_for_serdes -class SourceControlCodeReference(DagsterModel): - """Represents a source code location .""" +class UrlCodeReference(DagsterModel): + """Represents a source location which points at a URL, for example + in source control. + """ source_control_url: str - line_number: int label: Optional[str] = None @@ -64,7 +65,7 @@ class CodeReferencesMetadataValue(DagsterModel, MetadataValue["CodeReferencesMet references to source control. """ - code_references: List[Union[LocalFileCodeReference, SourceControlCodeReference]] + code_references: List[Union[LocalFileCodeReference, UrlCodeReference]] @property def value(self) -> "CodeReferencesMetadataValue": @@ -124,9 +125,7 @@ def _with_code_source_single_definition( existing_source_code_metadata = CodeReferencesMetadataSet.extract( metadata_by_key.get(key, {}) ) - sources_for_asset: List[ - Union[LocalFileCodeReference, SourceControlCodeReference] - ] = [ + sources_for_asset: List[Union[LocalFileCodeReference, UrlCodeReference]] = [ *existing_source_code_metadata.code_references.code_references, *sources, ] @@ -147,14 +146,13 @@ def convert_local_path_to_source_control_path( base_source_control_url: str, repository_root_absolute_path: str, local_path: LocalFileCodeReference, -) -> SourceControlCodeReference: +) -> UrlCodeReference: source_file_from_repo_root = os.path.relpath( local_path.file_path, repository_root_absolute_path ) - return SourceControlCodeReference( - source_control_url=f"{base_source_control_url}/{source_file_from_repo_root}", - line_number=local_path.line_number, + return UrlCodeReference( + source_control_url=f"{base_source_control_url}/{source_file_from_repo_root}#L{local_path.line_number}", label=local_path.label, ) @@ -178,7 +176,7 @@ def _convert_local_path_to_source_control_path_single_definition( existing_source_code_metadata = CodeReferencesMetadataSet.extract( metadata_by_key.get(key, {}) ) - sources_for_asset: List[Union[LocalFileCodeReference, SourceControlCodeReference]] = [ + sources_for_asset: List[Union[LocalFileCodeReference, UrlCodeReference]] = [ convert_local_path_to_source_control_path( base_source_control_url, repository_root_absolute_path, @@ -215,11 +213,30 @@ def link_to_source_control( source_control_branch: str, repository_root_absolute_path: Union[Path, str], ) -> Sequence[Union["AssetsDefinition", "SourceAsset", "CacheableAssetsDefinition"]]: + """Wrapper function which converts local file path code references to source control URLs + based on the provided source control URL and branch. + + Args: + assets_defs (Sequence[Union[AssetsDefinition, SourceAsset, CacheableAssetsDefinition]]): + The asset definitions to which source control metadata should be attached. + Only assets with local file code references (such as those created by + `with_source_code_references`) will be converted. + source_control_url (str): The base URL for the source control system. For example, + "https://github.com/dagster-io/dagster". + source_control_branch (str): The branch in the source control system, such as "master". + repository_root_absolute_path (Union[Path, str]): The absolute path to the root of the + repository on disk. This is used to calculate the relative path to the source file + from the repository root and append it to the source control URL. + """ if "gitlab.com" in source_control_url: source_control_url = _build_gitlab_url(source_control_url, source_control_branch) - else: - # assume GitHub URL scheme + elif "github.com" in source_control_url: source_control_url = _build_github_url(source_control_url, source_control_branch) + else: + raise ValueError( + "Invalid `source_control_url`." + " Only GitHub and GitLab are supported for linking to source control at this time." + ) return [ _convert_local_path_to_source_control_path_single_definition( @@ -235,7 +252,8 @@ def link_to_source_control( def with_source_code_references( assets_defs: Sequence[Union["AssetsDefinition", "SourceAsset", "CacheableAssetsDefinition"]], ) -> Sequence[Union["AssetsDefinition", "SourceAsset", "CacheableAssetsDefinition"]]: - """Wrapper function which attaches source code metadata to the provided asset definitions. + """Wrapper function which attaches local code reference metadata to the provided asset definitions. + This points to the filepath and line number where the asset body is defined. Args: assets_defs (Sequence[Union[AssetsDefinition, SourceAsset, CacheableAssetsDefinition]]): diff --git a/python_modules/dagster/dagster_tests/asset_defs_tests/test_asset_defs_source_metadata.py b/python_modules/dagster/dagster_tests/asset_defs_tests/test_asset_defs_source_metadata.py index 32dfc126ac0f3..6cc00f4c47ca1 100644 --- a/python_modules/dagster/dagster_tests/asset_defs_tests/test_asset_defs_source_metadata.py +++ b/python_modules/dagster/dagster_tests/asset_defs_tests/test_asset_defs_source_metadata.py @@ -4,7 +4,7 @@ from dagster import AssetsDefinition, load_assets_from_modules from dagster._core.definitions.metadata import ( LocalFileCodeReference, - SourceControlCodeReference, + UrlCodeReference, link_to_source_control, with_source_code_references, ) @@ -125,15 +125,15 @@ def test_asset_code_origins_source_control() -> None: assert isinstance( asset.metadata_by_key[key]["dagster/code_references"].code_references[-1], - SourceControlCodeReference, + UrlCodeReference, ) meta = cast( - SourceControlCodeReference, + UrlCodeReference, asset.metadata_by_key[key]["dagster/code_references"].code_references[-1], ) assert meta.source_control_url == ( "https://github.com/dagster-io/dagster/tree/master/python_modules/dagster" + (expected_file_path[len(DAGSTER_PACKAGE_PATH) :]) + + f"#L{expected_line_number}" ) - assert meta.line_number == int(expected_line_number)