fix: update owlbot configs to copy generated samples by alicejli · Pull Request #8790 · googleapis/google-cloud-java

alicejli · 2022-11-15T17:52:12Z

Tested with local Owlbot invocation using the following command (using java-analyticshub as the example):

docker run --rm --user $(id -u):$(id -g) \
  -v $(pwd):/repo -v /tmp/googleapis-gen:/googleapis-gen -w /repo \
  --env HOME=/tmp \
  gcr.io/cloud-devrel-public-resources/owlbot-cli:latest copy-code \
  --source-repo=/googleapis-gen \
  --config-file=java-analyticshub/.OwlBot.yaml

generation/update_owlbot_config.sh

meltsufin

Please merge in main and ensure that all ci checks are green.

meltsufin

How did you apply the script? Did you run this?

for F in `find . -maxdepth 2 -name '.OwlBot.yaml'`; do sh generation/set_owlbot_config.sh $F; done

I see some small differences when I run this command on the branch.

Also, would you mind adding a test to generated_files_sync.yaml for this script?

generation/set_owlbot_config.sh

alicejli · 2022-11-28T22:10:22Z

How did you apply the script? Did you run this?
for F in `find . -maxdepth 2 -name '.OwlBot.yaml'`; do sh generation/set_owlbot_config.sh $F; done
I see some small differences when I run this command on the branch.

Yes, that's what I ran. I had to manually edit java-grafeas/.OwlBot.yaml and java-accesscontextmanager/.OwlBot.yaml because they had an exceptional pattern compared to the other ones. Were those the same ones you noticed differences in?

Also, would you mind adding a test to generated_files_sync.yaml for this script?

Sorry I'm having trouble finding where that file is - do you mind linking? Thank you!

alicejli · 2022-11-29T17:32:40Z

Chatted with Mike offline that the test should be added here: https://github.com/googleapis/google-cloud-java/blob/main/.github/workflows/generated_files_sync.yaml

alicejli · 2023-01-12T16:49:05Z

@meltsufin I chatted with @suztomo about this and it seemed the best way to test this was to run owlbot locally to confirm the correct directories get copied over. I did that and confirmed the updated .OwlbotYaml files are working as intended, but we're not sure of a good way to actually add a test for these files via a script.

Do you have an idea of how you'd like to add a test? Or is testing Owlbot locally on this PR sufficient? Thanks!

meltsufin · 2023-01-12T19:01:34Z

@meltsufin I chatted with @suztomo about this and it seemed the best way to test this was to run owlbot locally to confirm the correct directories get copied over. I did that and confirmed the updated .OwlbotYaml files are working as intended, but we're not sure of a good way to actually add a test for these files via a script.

Do you have an idea of how you'd like to add a test? Or is testing Owlbot locally on this PR sufficient? Thanks!

The manual test is good, but I was talking more about the kind of tests that generated_files_sync.yaml has, where you just run the script (generation/set_owlbot_config.sh) for all modules and verify that no files have changed as a side-effect.

alicejli · 2023-01-13T16:27:07Z

The manual test is good, but I was talking more about the kind of tests that generated_files_sync.yaml has, where you just run the script (generation/set_owlbot_config.sh) for all modules and verify that no files have changed as a side-effect.

Ahh I think I understand - I added a check to run set_owlbot_config.sh in generated_files_sync.yaml. Do you mind taking another look to see if that is sufficient?

meltsufin · 2023-01-13T17:23:12Z

generation/check_owlbot_config.sh

+# Checks that each module's .OwlbotYaml file is correctly configured according to the template set in `set_owlbot_config.sh`
+
+OWLBOT_FILE=$1
+for F in $(find . -maxdepth 2 -name '.OwlBot.yaml'); do sh generation/set_owlbot_config.sh "$F"; done


Since this actually modifies the files, and doesn't really check, I think the file name doesn't really make sense. Would it make sense to just add this loop to set_owlbot_config.sh and update places where it's used? I think we always want to apply this to all .OwlBot.yaml files.

Hmm I see what you're saying; I did a little more digging and it looks like set_owlbot_config.sh is actually run as part of merge_repository.sh (https://github.com/googleapis/google-cloud-java/blob/main/generation/merge_repository.sh#L121) which is run regularly as a workflow: https://github.com/googleapis/google-cloud-java/blob/main/.github/workflows/merge_repository.yaml#L22.

My sense is that actually would suffice as the check for the Owlbot.yaml files - what do you think?

That check doesn't apply to the entire repo and might go away in the future. I think the check your added is valuable beyond that.

Thanks for pointers offline - let me know if this update works!

This is good, but I think you need to update 2 places where this script is used: bootstrap.sh and merge_repository.sh

Ahhh good catch; I updated merge_repository.sh. I think bootstrap.sh doesn't exist anymore as it was renamed to merge_repository.sh here: https://github.com/googleapis/google-cloud-java/pull/8832/files#diff-b082d5538e9aa0d75efd9ec5915d2a9462bd1237209225fa95b8be79517c0a63

I see that you updated merge_repository.sh, but bootstrap.sh still needs an update.

Right, never mind! I think it's still in my local for some reason.

meltsufin · 2023-01-13T17:23:35Z

.github/workflows/generated_files_sync.yaml

      - name: Fail if there's any difference
        run: git --no-pager diff --exit-code

+  owlbot-yaml:


Yes, this is what I had in mind. Thanks!

product-auto-label bot added the samples Issues that are directly related to samples. label Nov 15, 2022

alicejli mentioned this pull request Nov 15, 2022

fix: update owlbot.yaml files to copy generated samples directory fro… #8768

Closed

alicejli marked this pull request as ready for review November 15, 2022 17:58

alicejli requested review from blakeli0 and suztomo November 15, 2022 17:59

meltsufin reviewed Nov 15, 2022

View reviewed changes

generation/update_owlbot_config.sh Outdated Show resolved Hide resolved

meltsufin requested changes Nov 17, 2022

View reviewed changes

meltsufin force-pushed the updateOwlBotYamls branch from da890b5 to 4ea11b6 Compare November 18, 2022 17:01

alicejli force-pushed the updateOwlBotYamls branch 2 times, most recently from 73577dd to 9ead407 Compare November 18, 2022 17:48

alicejli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 18, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 18, 2022

alicejli requested a review from meltsufin November 21, 2022 16:00

meltsufin requested changes Nov 21, 2022

View reviewed changes

generation/set_owlbot_config.sh Outdated Show resolved Hide resolved

generation/set_owlbot_config.sh Outdated Show resolved Hide resolved

alicejli added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 28, 2022

alicejli force-pushed the updateOwlBotYamls branch from 8d72152 to ba31a4a Compare November 28, 2022 20:47

alicejli added 10 commits November 28, 2022 15:50

fix: update owlbot configs to copy generated samples

2b8e5cc

manual fix for accesscontextmanager

15c2826

merge owlbot config scripts

991a8cc

update exceptional grafeas config

4feece0

invert grep in script

4c67dc2

fix: update owlbot configs to copy generated samples

3d883eb

manual fix for accesscontextmanager

9b8156d

merge owlbot config scripts

ce36f9b

update exceptional grafeas config

f3e143e

invert grep in script

0039f58

alicejli force-pushed the updateOwlBotYamls branch from ba31a4a to 0039f58 Compare November 28, 2022 20:52

edits to script and new routing yaml

4642481

alicejli added 2 commits January 11, 2023 14:09

Merge branch 'main' into updateOwlBotYamls

25328dd

update three more Owlbot yaml files

2716226

alicejli added 4 commits January 13, 2023 09:49

Merge branch 'main' into updateOwlBotYamls

e4d7275

remove .bak generated files

bc35ecf

adding test to run templated owlbot yaml check

f0b589d

add check script

ef1119a

alicejli requested a review from meltsufin January 13, 2023 16:25

alicejli removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 13, 2023

meltsufin requested changes Jan 13, 2023

View reviewed changes

alicejli added 3 commits January 18, 2023 12:04

update set_owlbot_config.sh to loop directly in itself

fdbc57b

Merge branch 'main' into updateOwlBotYamls

70bf653

update merge_repository.sh for updated set_owlbot_config

b05f060

meltsufin approved these changes Jan 18, 2023

View reviewed changes

alicejli merged commit 5a404a8 into main Jan 18, 2023

alicejli deleted the updateOwlBotYamls branch January 18, 2023 21:30

release-please bot mentioned this pull request Jan 18, 2023

chore: release main #8998

Merged

alicejli mentioned this pull request Jan 19, 2023

feat: [datalineage] new module for datalineage #8996

Merged

Conversation

alicejli commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

meltsufin left a comment

Choose a reason for hiding this comment

Uh oh!

meltsufin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alicejli commented Nov 28, 2022

Uh oh!

alicejli commented Nov 29, 2022

Uh oh!

alicejli commented Jan 12, 2023

Uh oh!

meltsufin commented Jan 12, 2023

Uh oh!

alicejli commented Jan 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alicejli commented Nov 15, 2022 •

edited

Loading

meltsufin left a comment •

edited

Loading