damccorm commented on code in PR #23088: URL: https://github.com/apache/beam/pull/23088#discussion_r990215510
########## playground/infrastructure/datastore_client.py: ########## @@ -45,23 +45,28 @@ def __str__(self): # Google Datastore documentation link: https://cloud.google.com/datastore/docs/concepts class DatastoreClient: """DatastoreClient is a datastore client for sending a request to the Google.""" + _datastore_client: datastore.Client def __init__(self): self._check_envs() - self._datastore_client = datastore.Client(namespace=DatastoreProps.NAMESPACE, project=Config.GOOGLE_CLOUD_PROJECT) + self._datastore_client = datastore.Client( + namespace=DatastoreProps.NAMESPACE, + project=Config.GOOGLE_CLOUD_PROJECT + ) def _check_envs(self): if Config.GOOGLE_CLOUD_PROJECT is None: raise KeyError("GOOGLE_CLOUD_PROJECT environment variable should be specified in os") if Config.SDK_CONFIG is None: raise KeyError("SDK_CONFIG environment variable should be specified in os") - def save_to_cloud_datastore(self, examples_from_rep: List[Example], sdk: Sdk): + def save_to_cloud_datastore(self, examples_from_rep: List[Example], sdk: Sdk, origin: Origin): """ Save examples, output and meta to datastore Args: - :param sdk: sdk from parameters :param examples_from_rep: examples from the repository for saving to the Cloud Datastore + :param sdk: sdk from parameters + :param origin: typed origin const PG_EXAMPLES | TB_EXAMPLES Review Comment: This can also be PG_USER or TB_USER, right? ########## .github/workflows/playground_deploy_examples.yml: ########## @@ -43,13 +46,19 @@ jobs: - name: install deps run: pip install -r requirements.txt working-directory: playground/infrastructure - - shell: pwsh - name: get Difference + - name: get Difference id: check_file_changed run: | - $diff = git diff --name-only ${{ github.event.before }} ${{ github.event.after }} - echo "$diff" - Write-Host "::set-output name=example_diff::$diff" + set -xeu + # define the base ref + BASE_REF=$GITHUB_BASE_REF + if [ -z "$BASE_REF" ]; then + BASE_REF=origin/master + elif [ "$BASE_REF" == "master" ]; then + BASE_REF=origin/master Review Comment: ```suggestion if [ -z "$BASE_REF" ] || [ "$BASE_REF" == "master" ]; then BASE_REF=origin/master ``` ########## playground/infrastructure/test_helper.py: ########## @@ -33,38 +33,38 @@ @mock.patch("helper._check_file") @mock.patch("helper.os.walk") def test_find_examples_with_valid_tag(mock_os_walk, mock_check_file): - mock_os_walk.return_value = [("/root", (), ("file.java",))] + mock_os_walk.return_value = [("/root/sub1", (), ("file.java",))] mock_check_file.return_value = False sdk = SDK_UNSPECIFIED - result = find_examples(work_dir="", supported_categories=[], sdk=sdk) + result = find_examples(root_dir="/root", subdirs=["sub1"], supported_categories=[], sdk=sdk) Review Comment: Could we add a test with multiple subdirectories? ########## playground/infrastructure/helper.py: ########## @@ -112,24 +113,27 @@ def find_examples(work_dir: str, supported_categories: List[str], """ has_error = False examples = [] - for root, _, files in os.walk(work_dir): - for filename in files: - filepath = os.path.join(root, filename) - error_during_check_file = _check_file( - examples=examples, - filename=filename, - filepath=filepath, - supported_categories=supported_categories, - sdk=sdk) - has_error = has_error or error_during_check_file + for subdir in subdirs: Review Comment: What's the consequence of failing like this? If its a problem, could we do something to make it more robust like check for each path if its a prefix of another path and exclude the second path if so? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
