fix(hermetic-build): obtain gapic-generator-java locally on release branch #2023
Conversation
library_generation/utilities.sh
Outdated
| if [[ "${gapic_generator_version}" == *"-SNAPSHOT" ]]; then | ||
| # release please branches will point to a non-snapshot that hasn't been | ||
| # published. We want to get the generator pom locally in this case | ||
| if [[ "${gapic_generator_version}" == *"-SNAPSHOT" ]] || [[ "$(is_release_branch)" == "true" ]]; then |
There was a problem hiding this comment.
Can we always try to download from local maven repo first regardless of the name, and fall back to maven central if not exist? We are going to have multiple release branches in the future and this would become hard to maintain.
Similarly, we would have the same problem in the main generation script, can we try to fix it there as well @JoeWang1127 ?
There was a problem hiding this comment.
thanks for the quick review!
I modified the generator download (pom & jar) to default to a local fetch before trying the external source
library_generation/utilities.sh
Outdated
| # copy a SNAPSHOT version from maven local repository. | ||
| copy_from "$HOME/.m2/repository/com/google/api/gapic-generator-java-pom-parent/${gapic_generator_version}/gapic-generator-java-pom-parent-${gapic_generator_version}.pom" \ | ||
| # first, try to fetch the generator pom locally | ||
| bash -c "source ${util_script_dir}/utilities.sh && copy_from \"$HOME/.m2/repository/com/google/api/gapic-generator-java-pom-parent/${gapic_generator_version}/gapic-generator-java-pom-parent-${gapic_generator_version}.pom\" \ |
There was a problem hiding this comment.
Why do we need source command here?
There was a problem hiding this comment.
I added this to load the functions into the secondary shell. This is all with the intent to capture the exit code in case the local search doesn't work.
There was a problem hiding this comment.
Since we always try to copy files locally before downloading from maven central, I think we can modify copy_from not failing but return a value to indicate whether copy is success or not.
Also, we need to modify the error message in download_fail to indicate the file is not found neither locally or in maven central should the download fail.
There was a problem hiding this comment.
Thanks for pointing this out, makes sense. I modified copy_from to return (echo) a value instead. I added a message to indicate an external repo attempt will be made in case it wasn't found locally. I also created a more generic function for the pom parent and the jar.
product-auto-label
bot
added
size: m
library_generation/utilities.sh
Outdated
| copy_from() { | ||
| local local_repo=$1 | ||
| local save_as=$2 | ||
| cp "${local_repo}" "${save_as}" || \ | ||
| download_fail "${save_as}" "maven local" | ||
| cp "${local_repo}" "${save_as}" && echo "false" || echo "true" |
There was a problem hiding this comment.
what does this line mean?
It seems to echo true if cp failed.
There was a problem hiding this comment.
I added a comment on top of the function. I will modify the comment for it to have a better explanation
There was a problem hiding this comment.
also added a variable to hold the result in order to improve readability
library_generation/utilities.sh
Outdated
| @@ -174,17 +165,20 @@ download_from() { | |||
| curl -LJ -o "${save_as}" --fail -m 30 --retry 2 "$url" || download_fail "${save_as}" "${repo}" | |||
| } | |||
|
|
|||
| # copies the specified file in $1 to $2 | |||
| # will return "false" if the copy was successful, otherwise true (to indicate | |||
There was a problem hiding this comment.
It maybe unintuitive to indicate a successful copy as false, though it's a minor issue.
There was a problem hiding this comment.
I inverted the logic and changed variable names accordingly
| if [[ "${local_fetch_successful}" == "false" ]];then | ||
| # download gapic-generator-java artifact from Google maven central mirror if not | ||
| # found locally | ||
| >&2 echo "${artifact} not found locally. Attempting a download from Maven Central" |
There was a problem hiding this comment.
Can we add logs for successful local fetch or Maven central download? Otherwise it may be misleading that this is the only log we see when we are downloading from Maven Central.
There was a problem hiding this comment.
thanks, I added a couple success messages for both local and remote fetch
|
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!
|
|
[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!
|
|
[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!
|
This prevents the failures from showcase tests on the release-please branch since it tries to fetch a non-published version (
2.26.0). The fix is to check whether we are on a release please branch as an additional condition to fetch the generator jar and pom locally.