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

fix build failure related to stale versions in the cache #1449

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Sep 22, 2023

No description provided.

@@ -13,5 +13,5 @@ runs:

cd spring-cloud-kubernetes-integration-tests
# build the images, but dont run the tests
.././mvnw -T 1C clean install -DskipTests
.././mvnw -T 1C clean install -nsu -DskipTests
Copy link
Contributor Author

@wind57 wind57 Sep 22, 2023

Choose a reason for hiding this comment

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

this is a fix that affects us in a totally different PR, but I want to have it in 3.0.x, so that you merge it to main and I take it from main.

-nsu means no-snapshot-update and it has bitten me in a PR where I renamed classes.

To be more precise, there is an integration test that looks like this:

public class Fabric8DiscoveryController {

	private final Fabric8KubernetesDiscoveryClient discoveryClient;

}

Now, in the branch where I am working, Fabric8KubernetesDiscoveryClient exists, but in main it is called: KubernetesDiscoveryClient.

What happens is that the version that contains KubernetesDiscoveryClient is already published in the spring repo as a SNAPSHOT, so without -nsu here, it will go and download the version that will fail compilation. We need Fabric8KubernetesDiscoveryClient, but get KubernetesDiscoveryClient.

I hope it makes sense.

P.S. I don't understand why it did not happen until now, because that branch had a lot of successful builds, I assume you only recently started to publish 3.1.0-SNAPSHOT.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you saying its pulling down a snapshot version from the wrong branch?

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, basically I need my own version that I build during the build phase, but without -nsu it's pulling the version that exists in spring snapshot repos.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well then there is something else that is wrong then because it at the very least it should be pulling the version for the branch being built not a different version.

Copy link
Contributor Author

@wind57 wind57 Sep 24, 2023

Choose a reason for hiding this comment

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

first I have to apologize: the fact that I did not understand 100% of what happens, rushed me into a PR that would have only partially solve the issue. I spent some time into figuring it out and now I can present my case.

Here it is:

  1. we use a cache of maven .m2 directory for obvious reasons.
  2. before we build anything, we download that cache.

Now suppose such a sequence of events:

  • in our cache we have a version of libX-3.1.0-SNAPSHOT that contains a class called: Fabric8KubernetesDiscoveryClient that in turn is needed by libY-3.1.0-SNAPSHOT.
  • because we cache such entries, in our github cache, we will have libX-3.1.0-SNAPSHOT, but it will have such a maven-metadata-local.xml
<?xml version="1.0" encoding="UTF-8"?>
<metadata modelVersion="1.1.0">
  <groupId>.....</groupId>
  <artifactId>..</artifactId>
  <versioning>
    <lastUpdated>20230920045240</lastUpdated>
    <snapshot>
      <localCopy>true</localCopy>
    </snapshot>
    <snapshotVersions>
      <snapshotVersion>
        <classifier>sources</classifier>
        <extension>jar</extension>
        <value>3.1.0-SNAPSHOT</value>
        <updated>20230920045240</updated>
      </snapshotVersion>
      <snapshotVersion>
        <classifier>javadoc</classifier>
        <extension>jar</extension>
        <value>3.1.0-SNAPSHOT</value>
        <updated>20230920045240</updated>
      </snapshotVersion>
      <snapshotVersion>
        <extension>jar</extension>
        <value>3.1.0-SNAPSHOT</value>
        <updated>20230920045240</updated>
      </snapshotVersion>
      <snapshotVersion>
        <extension>pom</extension>
        <value>3.1.0-SNAPSHOT</value>
        <updated>20230920045240</updated>
      </snapshotVersion>
    </snapshotVersions>
  </versioning>
  <version>3.1.0-SNAPSHOT</version>
</metadata>

This file decides if we should update our snapshots or not. Notice how lastUpdated is : 20230920045240, or simpler: 2023/09/20. So this is when we placed this file in our cache, and every time we restore the cache, this is what maven is going to know about it.

Now the problem is that this dependency might be too old, the actual dependency stored in spring snapshot repo might be newer. If it is indeed newer, then we will try to download it and this is happens in our case.

  • local cache has : libX-3.1.0-SNAPSHOT with Fabric8KubernetesDiscoveryClient
  • spring repo snapshots libX-3.1.0-SNAPSHOT without Fabric8KubernetesDiscoveryClient

spring repo dependency is newer and this is what turns out being used, compilation failing as such.

What this PR does is to build the project after the cache is downloaded, in this manner maven will use the locally build snapshot, always.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it wasn't that we were pulling the wrong version of the dependencies, its that the cached version was using what was coming from spring snapshots and not what was built by the build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly.

@wind57 wind57 changed the title fix add -nsu to a stage Sep 22, 2023
@wind57 wind57 marked this pull request as ready for review September 22, 2023 16:43
@wind57
Copy link
Contributor Author

wind57 commented Sep 22, 2023

@ryanjbaxter ready to be looked at..

@wind57 wind57 changed the title add -nsu to a stage fix build failure related to stale versions in the cache Sep 24, 2023
@wind57 wind57 marked this pull request as draft September 24, 2023 13:59
@wind57 wind57 marked this pull request as ready for review September 24, 2023 16:30
shell: bash
run: |
./mvnw install -B \
-Dskip.build.image=true \
-DskipTests -DskipITs \
-T 1C -q
-T 1C -U -q
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't -U tell maven to update dependencies?

Copy link
Contributor Author

@wind57 wind57 Sep 25, 2023

Choose a reason for hiding this comment

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

yes, but this is what we want here, since we already restored the cache at this point. So this just updates potentially libraries that are not part of our maven reactor, which is good. it's like we update spring-boot-XXX-SNAPSHOT in this step, but not spring-cloud-kubernetes-commons-SNAPSHOT, if it makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I will just point out there is the potential...due to bad timing that we could finish the build locally and install those snapshots but inbetween that time and this command we could publish new snapshots to repo.spring.io and pull down those as well (for k8s). Although the chance of that is quite small and I am not sure there is anything we can really do about it.

@ryanjbaxter ryanjbaxter added this to the 3.0.5 milestone Sep 25, 2023
@ryanjbaxter ryanjbaxter merged commit 95c181b into spring-cloud:3.0.x Sep 25, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants