Skip to content
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.

Sign up for GitHub

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

chore: Fixing release-please config generation script to handle snapshot PRs. #8642

Merged
merged 3 commits into from
Oct 20, 2022
Merged

chore: Fixing release-please config generation script to handle snapshot PRs. #8642

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1737fe
chore: Fixing config generation script.
ddixit14 Oct 19, 2022
9fbac14
chore: removing unnecessary variables.
ddixit14 Oct 19, 2022
7b7a88f
using module_current_version
ddixit14 Oct 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions generation/generate_release_please_config.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,10 @@ for path in $module_list; do
module_line=$(grep -E "^((google-.*|grafeas|gapic\-libraries)).*:[0-9]+\.[0-9]+\.[0-9]+.*:[0-9]+\.[0-9]+\.[0-9]+.*$" "${version_file}")

artifact_name=$(echo "${module_line}" | cut -d ":" -f1)
module_released_version=$(echo "${module_line}" | cut -d ":" -f2)
module_snapshot_version=$(echo "${module_line}" | cut -d ":" -f3)
module_current_version=$(echo "${module_line}" | cut -d ":" -f3)

# concatenating module name and module version
rp_manifest_line="${tab}\"${module_name}\": \"${module_released_version}\""
Copy link
Member

Choose a reason for hiding this comment

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

So, module_released_version is no longer needed at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, correct. I just removed it.

Copy link
Contributor

@lqiu96 lqiu96 Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I thought the release-please manifest should always have the latest released version. Wouldn't this potentially change the versions in the manifest to a snapshot version depending on when this gets run in the future?

I could be wrong, but I was imaging this flow (maybe we have another script to fix this -- I may be missing something):

  1. We've run release:main and now have {release_version}:{current_version-SNAPSHOT}
  2. Add new library in monorepo and we need to generate a new manifest with the library
  3. Run this script

Manifest now has all the libraries with -SNAPSHOT since it's pulling current version.

Copy link
Contributor Author

@ddixit14 ddixit14 Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding point no.1, In the release:main branch, the versions will be like {release_version}:{release_version}.
At this point, our script will be generating non-snapshot versions. This is coherent with non-snapshot versions in manifest file.

We release, then the main will have {release_version}:{release_version}. All good till now.

Then we have snapshot PR. This PR is changing versions from {release_version}:{release_version} to {release_version}:{current_version-SNAPSHOT} in versions.txt. Also, the manifest file will change versions to snapshot too. We merge this in. once merged, our main will have {release_version}:{current_version-SNAPSHOT} in versions.txt and snapshot versions in manifest.

Next we have a new library in monorepo. We run the script to generate the config again. Our config will have snapshot versions in it. The next release:main PR will change the versions from snapshot to non-snapshot in manifest file, which is what we need to release.

This is how it should ideally work, in my understanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also when I think about it, if release-please does not update the manifest file in the snapshot PR, that would be the best solution.

Copy link
Contributor

@lqiu96 lqiu96 Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah gotcha, I didn't know the manifest also got updated to -SNAPSHOT. I thought it was the source of truth for each module's last released version and only held the latest released versions.

I think I understand the flow now, but correct me if I'm wrong:
For any new modules, the versions.txt file is 0.0.0:0.0.1-SNAPSHOT. This would mean that if the manifest is generated again with new module, it would use {current_version} which is 0.0.1-SNAPSHOT. So going forward, we would add in new modules ONLY after the release-please -SNAPSHOT PR has been merged in (since every module would now have -SNAPSHOT in the manifest). Then we can run the release:main PR for the actual release.

Copy link
Member

Choose a reason for hiding this comment

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

In java-cloud-bom repository (which uses manifest-based configuration) the ".release-please-manifest.json" has snapshot versions https://github.com/googleapis/java-cloud-bom/blob/main/.release-please-manifest.json

Copy link
Contributor Author

@ddixit14 ddixit14 Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

"So going forward, we would add in new modules ONLY after the release-please -SNAPSHOT PR has been merged in" -> Ideally, as soon as a release is done, the snapshot PR is created and should be merged in. Only after that, additional work for the next release will be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, makes sense given the -SNAPSHOT flow.

rp_manifest_line="${tab}\"${module_name}\": \"${module_current_version}\""
# Generate the JSON block with formatting
rp_config_line+="${tab}${tab}\"${module_name}\": {\n${tab}${tab}${tab}\"component\": \"${artifact_name}\"\n${tab}${tab}}"

Expand Down