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

Jump to bottom

ci: add downstream compatibility checks #1909

Merged
merged 13 commits into from
Aug 17, 2023
Merged

ci: add downstream compatibility checks #1909

merged 13 commits into from
Aug 17, 2023

Conversation

burkedavison
Copy link
Contributor

@burkedavison burkedavison commented Aug 7, 2023 •
edited
Loading

Adds a new GitHub workflow to perform downstream testing on client libraries.

  1. Installs sdk-platform-java to local maven.
  2. Checks out the downstream repository at the latest release associated with the main branch, not HEAD.
  3. Updates all dependencies on java-shared-dependency to the local version.
  4. Compiles and invokes unit tests for the given client libraries.

Does not invoke integration tests. Downstream integration testing is limited to google-cloud-java via the existing GraalVM Kokoro presubmits.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Aug 7, 2023
@burkedavison burkedavison added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed size: m Pull request size is medium. labels Aug 7, 2023
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Aug 7, 2023
@burkedavison burkedavison added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Aug 8, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 8, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 8, 2023
@burkedavison burkedavison marked this pull request as ready for review August 8, 2023 16:31
@burkedavison burkedavison requested a review from a team as a code owner August 8, 2023 16:31
@burkedavison burkedavison removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 8, 2023
meltsufin
meltsufin approved these changes Aug 8, 2023
View reviewed changes
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

It looks good, but would it make sense to add some tests for the functions added in common.sh?

@blakeli0
Copy link
Collaborator

blakeli0 commented Aug 8, 2023

  1. Checks out the downstream repository at the latest release associated with the main branch, not HEAD.

What's the reason to not checkout HEAD on main branch? To avoid possible frictions?

blakeli0
blakeli0 reviewed Aug 8, 2023
View reviewed changes
.kokoro/presubmit/downstream-build.sh
if [ -z "${MODULES_UNDER_TEST}" ]; then
echo "MODULES_UNDER_TEST must be set to run downstream-build.sh"
exit 1
fi
# Use default value for REPOS_UNDER_TEST if unset. If set to empty string, maintain empty string.
REPOS_UNDER_TEST=${REPOS_UNDER_TEST-"java-storage"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it intentional to have java-storage here?

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. It's the only current repository with GraalVM tests that don't require cloud project resources.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Can we add comments to explain it?

.kokoro/presubmit/downstream-compatibility.sh
source "$scriptDir/common.sh"

echo "Setup maven mirror"
mkdir -p "${HOME}/.m2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might be missing something, I thought .m2 folders would be created automatically by running mvn command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likely, however we haven't yet invoked mvn at this point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the content in settings.xml? If we don't push to anywhere, I think we might be able to just use the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/googleapis/sdk-platform-java/blob/main/settings.xml

It uses the Google mirror instead of maven central directly.

This logic is not new to this PR. If we remove it from here, we should remove use of the mirror in all the repos that use it. I'd ask this discussion not be considered part of this PR.

https://github.com/googleapis/java-shared-config/blob/763419aefd7efb40b30641641198ca6491b380d2/.kokoro/presubmit/downstream-build.sh#L42-L44

sdk-platform-java/.kokoro/presubmit/downstream-build.sh

Lines 28 to 30 in 33e40ff

# Use GCP Maven Mirror
mkdir -p "${HOME}/.m2"
cp settings.xml "${HOME}/.m2"

https://github.com/googleapis/google-cloud-java/blob/a082d632ddb3aa413886b731e0b0298f2bec8170/.kokoro/build.sh#L26-L28

Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic is not new to this PR. If we remove it from here, we should remove use of the mirror in all the repos that use it. I'd ask this discussion not be considered part of this PR.

Agreed. It was mostly for my own understanding, @suztomo what's the benefit of using Google mirror? I guess it's more stable than Maven central when downloading dependencies locally?

Copy link
Member

Choose a reason for hiding this comment

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

It's also faster in Kokoro CI because both are in Google'a infrastructure.

@burkedavison
Copy link
Contributor Author

What's the reason to not checkout HEAD on main branch? To avoid possible frictions?

  • To avoid any issues with main not always being stable. Across all repos, it's possible that main is not always ready to release and may enter into a bad state. OTOH, it's very unlikely that a release commit is in a bad state.
  • To make it more difficult to introduce a breaking change by updating sdk-platform-java and the downstream repo at the same time. It's still possible that the downstream repo prepares for the breaking change, releases, then makes the sdk-platform-java breaking change -- but less likely without triggering a conversation.

blakeli0
blakeli0 approved these changes Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

LGTM. Pending resolution of add more comments.

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Aug 9, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 9, 2023

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Aug 9, 2023

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Aug 9, 2023

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

meltsufin
meltsufin approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests!

@burkedavison burkedavison merged commit f86eca2 into main Aug 17, 2023
36 checks passed
@burkedavison burkedavison deleted the downstream-compatibility branch August 17, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suztomo suztomo suztomo left review comments

@blakeli0 blakeli0 blakeli0 approved these changes

@meltsufin meltsufin meltsufin approved these changes

Assignees
No one assigned
Labels
size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@burkedavison @blakeli0 @suztomo @meltsufin @yoshi-kokoro