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.

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

Conversation

ddixit14
Copy link
Contributor

@ddixit14 ddixit14 commented Oct 19, 2022

This will handle the case for config to have snapshots in it. Till now we were generating config only with non-snapshot versions.

Ideal Results for this script:

  • For release:main PR, this script will generate config with no-snapshots in it (in release:main PR, the manifest file has non-snapshot versions)

  • For release:snapshot PR, this script will generate config with snapshots in it (in snapshot PR, the manifest file has snapshot versions)

This will handle the case for config to have snapshots in it. Till now we were generating config only with non-snapshot versions.

Ideal Results for this script:
For release:main PR, this script will generate config with no-snapshots in it (in release:main PR, the manifest file has non-snapshot versions)
For release:snapshot PR, this script will generate config with snapshots in it (in snapshot PR, the manifest file has snapshot versions)
@@ -30,7 +30,7 @@ for path in $module_list; do
module_snapshot_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

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

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

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

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename it to module_current_version? The 3rd column is marked as current.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@meltsufin meltsufin merged commit ca7e864 into main Oct 20, 2022
@meltsufin meltsufin deleted the ddixit14-patch-2 branch October 20, 2022 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants