[deploy_maven] allow for release repo override via command line#350
Open
sugarmanz wants to merge 1 commit intotypedb:masterfrom
Open
[deploy_maven] allow for release repo override via command line#350sugarmanz wants to merge 1 commit intotypedb:masterfrom
sugarmanz wants to merge 1 commit intotypedb:masterfrom
Conversation
allow grouping releases into a single staging repo
alexjpwalker
requested changes
Nov 23, 2022
Comment on lines
+319
to
+321
| release_repo = ctx.attr.release | ||
| if _DEPLOY_MAVEN_RELEASE_REPO_KEY in ctx.var: | ||
| release_repo = ctx.var[_DEPLOY_MAVEN_RELEASE_REPO_KEY] |
Member
There was a problem hiding this comment.
I agree with the approach of using --define! This is aligned with how we override the version being deployed:
bazel run --define version=$(cat VERSION) //:deploy-maven -- releaseA couple of points:
- Since we're allowing users to override the release repo, we should also allow them to override the snapshot repo.
- As
versionis all lowercase, so should bedeploy_maven_release_repo; and for terseness, I'd call it justrelease_repo. (As this is aruntarget, the key need not be globally unique.)
With all that in mind, and simplifying the Starlark a little:
Suggested change
| release_repo = ctx.attr.release | |
| if _DEPLOY_MAVEN_RELEASE_REPO_KEY in ctx.var: | |
| release_repo = ctx.var[_DEPLOY_MAVEN_RELEASE_REPO_KEY] | |
| snapshot_repo = ctx.var.get("snapshot_repo", default=ctx.attr.snapshot) | |
| release_repo = ctx.var.get("release_repo", default=ctx.attr.release) |
(requires additional update to L330: "{snapshot}": snapshot_repo,)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the goal of this PR?
Allow for the release repo to be configured dynamically. With Maven Central, there is the opportunity to batch a release with a singular staging repository. Unfortunately, without some rework, it would be a little difficult to dynamically set the release repository. I don't really want to dynamically touch the
BUILDor.bzlfiles where I might have therelease_repodefined that is passed into the rule since that could differ between repos. This change allows therelease_repoto be overridden from a CLI such that the staging repository can be created dynamically during CI and passed to the deploy target for publishing before dynamically closing & releasing the staging repo.Related shell script
What are the changes implemented in this PR?
This is an interesting PR b/c there are a few ways to solve this. I could have modified the
.pyscript to try and read from an env var. However, given that this is really a one-off URL, the potential for a stale repo being stored and used from the env was non-zero. Having to manually specify the staging repo w/in the bazel invocation seemed like a good middleground.Opinions appreciated 😄