Skip to content

chore: re-generate when config or Dockerfile is changed #3146

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

Merged
merged 4 commits into from
Sep 3, 2024

Conversation

JoeWang1127
Copy link
Collaborator

In this PR:

  • Re-generate common protos and iam if generation config or Dockerfile is changed.

I use #3145 to verify the change.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Aug 30, 2024
@JoeWang1127 JoeWang1127 marked this pull request as ready for review August 30, 2024 20:46
gcr.io/cloud-devrel-public-resources/java-library-generation:"${image_tag}" \
--baseline-generation-config-path="${workspace_name}/${baseline_generation_config}" \
--current-generation-config-path="${workspace_name}/${generation_config}"
gcr.io/cloud-devrel-public-resources/java-library-generation:"${image_tag}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the commit history going to be populated correctly if we don't pass the baseline generation config into the image?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the commit history has to go through baseline/current config comparison.

However, in this repo, we don't have config changes other than googleapis commit since everything else is baked in the image.

git add --all -- ':!pr_description.txt'
changed_files=$(git diff --cached --name-only)
if [[ "${changed_files}" == "" ]]; then
echo "There is no generated code change with the generation config and Dockerfile change ${config_diff}."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually mentioned Dockerfile change in the message but it was not achieved before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dockerfile is not part of generation config, so the previous docker run won't generate anything if Dockerfile is the only change.

@JoeWang1127 JoeWang1127 requested a review from blakeli0 August 30, 2024 21:03
Copy link
Contributor

@diegomarquezp diegomarquezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. IIUC this is more generic than only allowing changes in the Dockerfile. I think this can allow modifications coming from anything that alters the output of the generation: postprocessing, gapic generator, Dockerfile, etc. And it's great to allow this btw, but maybe the title/description should be something like "re-generate when config or generation workflow has changed"?

@diegomarquezp
Copy link
Contributor

diegomarquezp commented Sep 3, 2024

LGTM. IIUC this is more generic than only allowing changes in the Dockerfile. I think this can allow modifications coming from anything that alters the output of the generation: postprocessing, gapic generator, Dockerfile, etc. And it's great to allow this btw, but maybe the title/description should be something like "re-generate when config or generation workflow has changed"?

From internal discussion with @JoeWang1127, this is wrong. There was a modification on what would allow the generation to start made in 74f20a2. So only the Dockerfile or the config yaml would trigger this.

Copy link

sonarqubecloud bot commented Sep 3, 2024

Copy link

sonarqubecloud bot commented Sep 3, 2024

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@JoeWang1127 JoeWang1127 merged commit fa7c808 into main Sep 3, 2024
48 of 49 checks passed
@JoeWang1127 JoeWang1127 deleted the chore/change-gen-strategy branch September 3, 2024 16:09
ldetmer pushed a commit that referenced this pull request Sep 17, 2024
In this PR:
- Re-generate common protos and iam if generation config or Dockerfile
is changed.

I use #3145 to verify the change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants