Owlbot should keep track of custom modifications to templates #1798

kolea2 · 2023-05-02T18:01:55Z

We have some examples of libraries that make modifications to otherwise standard templates. For example, https://github.com/googleapis/java-bigtable/blob/main/.kokoro/presubmit/integration.cfg#L9-L12 adds custom IT args to this standard template: https://github.com/googleapis/synthtool/blob/master/synthtool/gcp/templates/java_library/.kokoro/presubmit/integration.cfg. As such, we need to exclude it from owlbot and any new iterations of these templates need to be handled manually (see googleapis/java-bigtable#1255 and fix googleapis/java-bigtable#1258).

It would be great if owlbot could preserve known additions to templates so we can still receive updates without having to do manual work on them in a repeated way.

SurferJeffAtGoogle · 2023-05-08T16:44:43Z

If you add integration.cfg to the list of excludes here
https://github.com/googleapis/java-bigtable/blob/722c6c58e42c51ac414a7e189c711e844447b9ce/owlbot.py#L98
then Owl Bot won't touch it. Will that satisfy your needs?

kolea2 · 2023-05-09T14:29:11Z

integration.cfg is already excluded. I would like to not exclude any templates, but have owlbot insert custom arguments specific to the library (in this case, INTEGRATION_TEST_ARGS).

SurferJeffAtGoogle · 2023-05-10T21:22:49Z

I see. That logic can be coded into owlbot.py and synthtool. In other words, it's a feature request for your post processor (https://github.com/googleapis/synthtool).

kolea2 · 2023-05-11T14:50:43Z

Transferring to appropriate repo and reopening.

suztomo · 2023-05-24T18:30:51Z

The problem is that the granularity of owlbot.py exclusion logic (implemented by java.common_templates(excludes=[ ... (usage link) is file-level and this is too coarse grained to meet this issue.

Rough idea 1: Somehow we use Git's merging capability.

Rough idea 2: INTEGRATION_TEST_ARGS to be in independent file that the build.sh reads if one exists.

I'll think more.

kolea2 · 2023-05-25T18:25:15Z

@suztomo just building off of your second idea a bit - something similar to https://github.com/googleapis/java-bigtable/blob/main/.readme-partials.yml which inserts custom things into our READMEs could work here as well. Would also work nicely if new environment variables need to be introduced in the future.

suztomo · 2023-05-26T14:53:52Z

Found that https://github.com/googleapis/synthtool/blob/master/synthtool/gcp/partials.py Thank you for information.

Is INTEGRATION_TEST_ARGS the only thing we should worry about?

This is the list of files excluded from templating.

java.common_templates(excludes=[
    '.gitignore',
    '.kokoro/presubmit/integration.cfg',
    '.kokoro/presubmit/graalvm-native.cfg',
    '.kokoro/presubmit/graalvm-native-17.cfg',
    '.kokoro/nightly/integration.cfg',
    '.kokoro/presubmit/samples.cfg',
    '.kokoro/nightly/samples.cfg',

suztomo@suztomo2:~/synthtool$ for N in presubmit/integration.cfg presubmit/graalvm-native.cfg presubmit/graalvm-native-17.cfg nightly/integration.cfg presubmit/samples.cfg nightly/samples.cfg; do diff --unified=0 ~/java-bigtable/.kokoro/${N} ~/synthtool/synthtool/gcp/templates/java_library/.kokoro/${N}; done
--- /usr/local/google/home/suztomo/java-bigtable/.kokoro/presubmit/integration.cfg	2022-06-27 10:45:58.426916784 -0400
+++ /usr/local/google/home/suztomo/synthtool/synthtool/gcp/templates/java_library/.kokoro/presubmit/integration.cfg	2022-05-17 12:20:33.234220468 -0400
@@ -10,5 +9,0 @@
-    key: "INTEGRATION_TEST_ARGS"
-    value: "-P bigtable-emulator-it,bigtable-prod-it,bigtable-prod-batch-it -Dbigtable.project=gcloud-devel -Dbigtable.instance=google-cloud-bigtable -Dbigtable.table=integration-tests -Dbigtable.kms_key_name=projects/gcloud-devel/locations/us-east1/keyRings/cmek-test-key-ring/cryptoKeys/cmek-test-key -Dbigtable.wait-for-cmek-key-status=true"
-}
-
-env_vars: {
--- /usr/local/google/home/suztomo/java-bigtable/.kokoro/presubmit/graalvm-native.cfg	2023-05-19 10:53:32.462094047 -0400
+++ /usr/local/google/home/suztomo/synthtool/synthtool/gcp/templates/java_library/.kokoro/presubmit/graalvm-native.cfg	2023-05-18 13:24:15.185686922 -0400
@@ -21,5 +20,0 @@
-    key: "INTEGRATION_TEST_ARGS"
-    value: "-P bigtable-emulator-it,bigtable-prod-it,bigtable-prod-batch-it -Dbigtable.project=gcloud-devel -Dbigtable.instance=google-cloud-bigtable -Dbigtable.table=integration-tests -Dbigtable.kms_key_name=projects/gcloud-devel/locations/us-east1/keyRings/cmek-test-key-ring/cryptoKeys/cmek-test-key -Dbigtable.wait-for-cmek-key-status=true"
-}
-
-env_vars: {
--- /usr/local/google/home/suztomo/java-bigtable/.kokoro/presubmit/graalvm-native-17.cfg	2023-05-19 10:53:32.462094047 -0400
+++ /usr/local/google/home/suztomo/synthtool/synthtool/gcp/templates/java_library/.kokoro/presubmit/graalvm-native-17.cfg	2023-05-18 13:24:15.185686922 -0400
@@ -14,5 +13,0 @@
-env_vars: {
-    key: "INTEGRATION_TEST_ARGS"
-    value: "-P bigtable-emulator-it,bigtable-prod-it,bigtable-prod-batch-it -Dbigtable.project=gcloud-devel -Dbigtable.instance=google-cloud-bigtable -Dbigtable.table=integration-tests -Dbigtable.kms_key_name=projects/gcloud-devel/locations/us-east1/keyRings/cmek-test-key-ring/cryptoKeys/cmek-test-key -Dbigtable.wait-for-cmek-key-status=true"
-}
-
@@ -38 +33 @@
-}
+}
\ No newline at end of file
--- /usr/local/google/home/suztomo/java-bigtable/.kokoro/nightly/integration.cfg	2022-06-27 10:45:58.426916784 -0400
+++ /usr/local/google/home/suztomo/synthtool/synthtool/gcp/templates/java_library/.kokoro/nightly/integration.cfg	2022-11-01 13:39:05.609656779 -0400
@@ -10,5 +9,0 @@
-    key: "INTEGRATION_TEST_ARGS"
-    value: "-P bigtable-emulator-it,bigtable-prod-it,bigtable-prod-batch-it -Dbigtable.project=gcloud-devel -Dbigtable.instance=google-cloud-bigtable -Dbigtable.table=integration-tests -Dbigtable.kms_key_name=projects/gcloud-devel/locations/us-east1/keyRings/cmek-test-key-ring/cryptoKeys/cmek-test-key -Dbigtable.wait-for-cmek-key-status=true"
-}
-
-env_vars: {
@@ -21 +16 @@
-  value: "gcloud-devel"
+  value: "java-docs-samples-testing"
@@ -26 +21 @@
-  value: "gcloud-devel"
+  value: "java-docs-samples-testing"
@@ -30,2 +25,2 @@
-  key: "ENABLE_BUILD_COP"
-  value: "true"
+  key: "ENABLE_FLAKYBOT"
+  value: {% if migrated_split_repo %}"false"{% else %}"true"{% endif %}
--- /usr/local/google/home/suztomo/java-bigtable/.kokoro/presubmit/samples.cfg	2022-06-27 10:45:58.430916747 -0400
+++ /usr/local/google/home/suztomo/synthtool/synthtool/gcp/templates/java_library/.kokoro/presubmit/samples.cfg	2022-05-17 12:20:33.238220432 -0400
@@ -33,5 +32,0 @@
-}
-
-env_vars: {
-  key: "BIGTABLE_TESTING_INSTANCE"
-  value: "instance"
--- /usr/local/google/home/suztomo/java-bigtable/.kokoro/nightly/samples.cfg	2022-06-27 10:45:58.426916784 -0400
+++ /usr/local/google/home/suztomo/synthtool/synthtool/gcp/templates/java_library/.kokoro/nightly/samples.cfg	2022-11-01 13:39:05.613656740 -0400
@@ -36,2 +36,2 @@
-  key: "ENABLE_BUILD_COP"
-  value: "true"
+  key: "ENABLE_FLAKYBOT"
+  value: {% if migrated_split_repo %}"false"{% else %}"true"{% endif %}
@@ -39,5 +38,0 @@
-
-env_vars: {
-  key: "BIGTABLE_TESTING_INSTANCE"
-  value: "instance"
-}
\ No newline at end of file

suztomo · 2023-06-06T17:17:00Z

In addition to INTEGRATION_TEST_ARGS, there are other environment variables that differ from the template: gcloud-devel BIGTABLE_TESTING_INSTANCE.

Now I'm feeing a solution similar to .readme-partials.yml is better to accommodate different needs

kolea2 · 2023-06-06T17:44:37Z

@suztomo once googleapis/java-datastore#1102 is merged, we will also have DATASTORE_PROJECT_ID. All to say, yes - there are env variables other than INTEGRATION_TEST_ARGS (and new ones can always be added in the future).

suztomo · 2023-06-06T19:27:25Z

@kolea2 Got it.

Idea of partial files for env_vars

.kokoro/nightly/.samples.cfg.partials.yml. (In general, its .<cfg file name>.partials.yml in the same directory). The YAML file contains the following section:

env_vars:
- key: INTEGRATION_TEST_ARGS
  value: "-P bigtable-emulator-it,bigtable-prod-it,bigtable-prod-batch-it -Dbigtable.project=gcloud-devel -Dbigtable.instance=google-cloud-bigtable -Dbigtable.table=integration-tests -Dbigtable.kms_key_name=projects/gcloud-devel/locations/us-east1/keyRings/cmek-test-key-ring/cryptoKeys/cmek-test-key -Dbigtable.wait-for-cmek-key-status=true"
- key: DATASTORE_PROJECT_ID
  value: java-docs-samples-testing

When OwlBot post processor generates the templated file with ".cfg", it looks for .<cfg file name>.partials.yml in the GitHub repository. If the partial YAML file exists, then it does the following to the generated file:

If the template already has the key for env_vars , then it overrides the value.
If the template does not have the key for the env_vars, then it adds a new env_vars.

What about other fields than env_vars?

For now I focus on env_vars only. This design allows us to extend other fields than env_vars.

JoeWang1127 · 2023-09-23T14:28:52Z

@suztomo I found that load_partials is called in common_templates to load key-value pairs from partial files.

I can add an additional parameter to common_templates, called partial_files with default value, to load partials other than .readme-partials.yml or .readme-partials.yaml into the templates.

Then in owlbot.py, to load partials:

java.common_templates(
  excludes=[
    '.gitignore',
    '.kokoro/presubmit/integration.cfg',
    '.kokoro/presubmit/graalvm-native.cfg',
    '.kokoro/presubmit/graalvm-native-17.cfg',
  ],
  partial_files=[".kokoro/nightly/integration.cfg-partials.yaml"]
)

WDYT?

suztomo · 2023-09-25T03:48:24Z

@JoeWang1127 Good start. Before implementing it, would you write down what ".kokoro/nightly/integration.cfg-partials.yaml" would look like?

JoeWang1127 · 2023-09-25T14:33:52Z

This is a sample of .kokoro/nightly/integration.cfg-partials.yaml.
Everything from the 2nd line will be inserted into .kokoro/nightly/integration.cfg.

kokoro_nightly_integration: |
  env_vars: {
    key: "INTEGRATION_TEST_ARGS"
    value: "-P bigtable-emulator-it,bigtable-prod-it,bigtable-prod-batch-it -Dbigtable.project=gcloud-devel -Dbigtable.instance=google-cloud-bigtable -Dbigtable.table=integration-tests -Dbigtable.kms_key_name=projects/gcloud-devel/locations/us-east1/keyRings/cmek-test-key-ring/cryptoKeys/cmek-test-key -Dbigtable.wait-for-cmek-key-status=true"
  }

In the template, we need to add the following section:

{% if 'partials' in metadata and metadata['partials']['kokoro_nightly_integration'] %}
{{ metadata['partials']['kokoro_nightly_integration'] }}
{% endif %}

suztomo · 2023-09-25T14:36:35Z

@JoeWang1127 What about the difference between gcloud-devel proejct and java-docs-samples-testing project?

-  value: "gcloud-devel"
+  value: "java-docs-samples-testing"

JoeWang1127 · 2023-09-25T14:55:11Z

The contents of partials can only be inserted into templates, so two env_vars can't be merged even if they have the same key.

suztomo · 2023-09-25T15:10:21Z

Good. Then the current load_partials does not work well for the cfg files. Would you design something better? Start with an ideal integration.cfg-partials.yaml file.

JoeWang1127 · 2023-09-25T16:00:15Z

I think integration.cfg-partials.yaml can be:

kokoro_nightly_integration:
  - key: "INTEGRATION_TEST_ARGS"
    value: "-P bigtable-emulator-it,bigtable-prod-it,bigtable-prod-batch-it -Dbigtable.project=gcloud-devel -Dbigtable.instance=google-cloud-bigtable -Dbigtable.table=integration-tests -Dbigtable.kms_key_name=projects/gcloud-devel/locations/us-east1/keyRings/cmek-test-key-ring/cryptoKeys/cmek-test-key -Dbigtable.wait-for-cmek-key-status=true"
  - key: "GCLOUD_PROJECT"
    value: "gcloud-devel"

If the key is not presented in the template, then the key-value pair is inserted to the template; otherwise the value will be overwritten. Something like you mentioned.

The template needs redesign and a new file integration.cfg-default.yaml to hold default key-value pairs.

suztomo · 2023-09-25T16:18:39Z

It reads as if it only takes care of envvars implicitly.

JoeWang1127 · 2023-09-25T17:09:49Z

In the template, it says the format of integration.cfg is defined in //devtools/kokoro/config/proto/build.proto, but I can't find this file in google3.

If we need other keys besides env_vars, then integration.cfg-partials.yaml can be:

kokoro_nightly_integration:
    env_vars:
        - key: "INTEGRATION_TEST_ARGS"
          value: "-P bigtable-emulator-it,bigtable-prod-it,bigtable-prod-batch-it -Dbigtable.project=gcloud-devel -Dbigtable.instance=google-cloud-bigtable -Dbigtable.table=integration-tests -Dbigtable.kms_key_name=projects/gcloud-devel/locations/us-east1/keyRings/cmek-test-key-ring/cryptoKeys/cmek-test-key -Dbigtable.wait-for-cmek-key-status=true"
        - key: "GCLOUD_PROJECT"
          value: "gcloud-devel"

suztomo · 2023-09-25T18:50:51Z

I feel the 1st line "kokoro_nightly_integration:" is redundant, assuming integration.cfg-partials.yaml file is in .kokoro/nightly/ directory.

JoeWang1127 · 2023-09-25T19:06:46Z

I use kokoro_nightly_integration to specify all key-value pairs should go into .kokoro/nightly/integration.cfg.

I think I can parse the file name so that all key-value pairs in .kokoro/nightly/integration.cfg-partials.yaml can go into .kokoro/nightly/integration.cfg.

Then integration.cfg-partials.yaml is:

env_vars:
  - key: "INTEGRATION_TEST_ARGS"
    value: "-P bigtable-emulator-it,bigtable-prod-it,bigtable-prod-batch-it -Dbigtable.project=gcloud-devel -Dbigtable.instance=google-cloud-bigtable -Dbigtable.table=integration-tests -Dbigtable.kms_key_name=projects/gcloud-devel/locations/us-east1/keyRings/cmek-test-key-ring/cryptoKeys/cmek-test-key -Dbigtable.wait-for-cmek-key-status=true"
  - key: "GCLOUD_PROJECT"
    value: "gcloud-devel"

kolea2 · 2023-09-26T13:58:41Z

@JoeWang1127 thanks for digging into this!

One question - I noticed you're specifying the nightly directory, but we'd want this to take effect on all integration configurations (nighty, presubmit, nightly/java11-integration). Does this account for that?

JoeWang1127 · 2023-09-26T14:40:04Z

Hi @kolea2, you can specify customized env_vars for different configuration files, but you need to provide a partial file for each of the template and the partial file and template should be in the same directory.

For example, if you want to define env_vars for .kokoro/nightly/integration.cfg and .kokoro/presubmit/integration.cfg, you need to provide .kokoro/nightly/integration.cfg-partial.yaml and .kokoro/presubmit/integration.cfg-partial.yaml

I mainly focus on env_vars right now, but this design can be extended to add other keys, though we need to modify the template.

Does this sound good to you?

JoeWang1127 · 2023-09-28T17:05:04Z

Since I haven't heard more feedback, I'll start to implement as #1798 (comment)

kolea2 added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. priority: p3 Desirable enhancement or fix. May not be included in next release. labels May 2, 2023

blunderbuss-gcf bot assigned SurferJeffAtGoogle May 2, 2023

SurferJeffAtGoogle closed this as completed May 10, 2023

kolea2 transferred this issue from googleapis/repo-automation-bots May 11, 2023

kolea2 reopened this May 11, 2023

SurferJeffAtGoogle added the lang: java Issues specific to Java. label May 22, 2023

SurferJeffAtGoogle assigned suztomo and unassigned SurferJeffAtGoogle May 24, 2023

suztomo removed their assignment Sep 20, 2023

JoeWang1127 self-assigned this Sep 22, 2023

JoeWang1127 mentioned this issue Oct 3, 2023

chore: track customization in templates #1876

Merged

chingor13 closed this as completed in #1876 Oct 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Owlbot should keep track of custom modifications to templates #1798

Owlbot should keep track of custom modifications to templates #1798

kolea2 commented May 2, 2023

SurferJeffAtGoogle commented May 8, 2023

kolea2 commented May 9, 2023

SurferJeffAtGoogle commented May 10, 2023

kolea2 commented May 11, 2023

suztomo commented May 24, 2023 •

edited

Loading

kolea2 commented May 25, 2023

suztomo commented May 26, 2023 •

edited

Loading

suztomo commented Jun 6, 2023

kolea2 commented Jun 6, 2023 •

edited

Loading

suztomo commented Jun 6, 2023 •

edited

Loading

JoeWang1127 commented Sep 23, 2023

suztomo commented Sep 25, 2023

JoeWang1127 commented Sep 25, 2023

suztomo commented Sep 25, 2023

JoeWang1127 commented Sep 25, 2023

suztomo commented Sep 25, 2023 •

edited

Loading

JoeWang1127 commented Sep 25, 2023

suztomo commented Sep 25, 2023 •

edited

Loading

JoeWang1127 commented Sep 25, 2023

suztomo commented Sep 25, 2023

JoeWang1127 commented Sep 25, 2023 •

edited

Loading

kolea2 commented Sep 26, 2023

JoeWang1127 commented Sep 26, 2023

JoeWang1127 commented Sep 28, 2023

Owlbot should keep track of custom modifications to templates #1798

Owlbot should keep track of custom modifications to templates #1798

Comments

kolea2 commented May 2, 2023

SurferJeffAtGoogle commented May 8, 2023

kolea2 commented May 9, 2023

SurferJeffAtGoogle commented May 10, 2023

kolea2 commented May 11, 2023

suztomo commented May 24, 2023 • edited Loading

kolea2 commented May 25, 2023

suztomo commented May 26, 2023 • edited Loading

Is INTEGRATION_TEST_ARGS the only thing we should worry about?

suztomo commented Jun 6, 2023

kolea2 commented Jun 6, 2023 • edited Loading

suztomo commented Jun 6, 2023 • edited Loading

Idea of partial files for env_vars

What about other fields than env_vars?

JoeWang1127 commented Sep 23, 2023

suztomo commented Sep 25, 2023

JoeWang1127 commented Sep 25, 2023

suztomo commented Sep 25, 2023

JoeWang1127 commented Sep 25, 2023

suztomo commented Sep 25, 2023 • edited Loading

JoeWang1127 commented Sep 25, 2023

suztomo commented Sep 25, 2023 • edited Loading

JoeWang1127 commented Sep 25, 2023

suztomo commented Sep 25, 2023

JoeWang1127 commented Sep 25, 2023 • edited Loading

kolea2 commented Sep 26, 2023

JoeWang1127 commented Sep 26, 2023

JoeWang1127 commented Sep 28, 2023

suztomo commented May 24, 2023 •

edited

Loading

suztomo commented May 26, 2023 •

edited

Loading

kolea2 commented Jun 6, 2023 •

edited

Loading

suztomo commented Jun 6, 2023 •

edited

Loading

suztomo commented Sep 25, 2023 •

edited

Loading

suztomo commented Sep 25, 2023 •

edited

Loading

JoeWang1127 commented Sep 25, 2023 •

edited

Loading