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

[JENKINS-48061] Allow retrieve of a commit hash with no known branch or tag association #557

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@jglick
Copy link
Member

jglick commented Dec 1, 2017

JENKINS-48061

#551 regressed a valid use case. PCT found that ResourceStepTest.duplicatedResources in workflow-cps-global-lib passes against 3.6.3 but fails against 3.6.4.

@reviewbybees

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Dec 1, 2017

Nice! Glad that the plugin compatibility test is running.

I believe this is the fix for JENKINS-48061

@jglick jglick referenced this pull request Dec 1, 2017

Merged

Updates for PCT #42

@jglick jglick changed the title [JENKINS-47824] Allow retrieve of a commit hash with no known branch or tag association [JENKINS-48061] Allow retrieve of a commit hash with no known branch or tag association Dec 1, 2017

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Dec 1, 2017

I ran a test drive with this change and it was unable to find a non-tip revision that is in my pipeline shared library. I'm chasing more details currently. I'm glad it resolves the automated test, but I think additional changes will be needed before it will resolve the case described in JENKINS-48061

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Dec 1, 2017

So this is not how I want to fix this. There will be secondary issues as the hash will be treated as a branch name and cause knock-on issues elsewhere.

🐛

The correct non-hack fix will require adding a concrete SCMHead implementation class (actually two of them) and will also enable additional functionality to be deliverable by extension plugins

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Dec 1, 2017

FTR I have half of the real fix developed last week but this week with meetings I have been unable to finish the migration testing to ensure no spurious rebuilds

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Dec 1, 2017

BTW use the GH “request changes” gesture as appropriate.

the hash will be treated as a branch name and cause knock-on issues elsewhere

Potentially. In the observed use case of fetch(String, TaskListener) from workflow-cps-global-lib, the SCMHead.name is ignored anyway—all that counts is that the SCMRevision can be checked out after building the matching SCM. After all, it worked fine before you started returning null.

A new SCMHead implementation for something like “detached head” would indeed be the nice way to fix this, but how long is it going to take to develop? In the meantime we have a serious regression.

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Dec 1, 2017

I have it almost done... just had other stuff to do this week

jglick added a commit to jglick/workflow-cps-global-lib-plugin that referenced this pull request Dec 1, 2017

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Dec 6, 2017

#559 I presume? (Not a very helpful PR title!)

@jglick jglick referenced this pull request Dec 6, 2017

Closed

[JENKINS-48385] Migrate from SCMHead to concrete subclass #559

0 of 5 tasks complete
@stephenc
Copy link
Member

stephenc left a comment

Not the actual fix. Tests are just vanity tests and do not actually test the issue reported

@@ -426,6 +427,11 @@ public void retrieveRevision() throws Exception {
assertNull(fileAt("\n", run, source, listener));
assertThat(source.fetchRevisions(listener), hasItems("master", "dev", "v1"));
// we do not care to return commit hashes or other references
// Commit hashes which are not heads:

This comment has been minimized.

Copy link
@stephenc

stephenc Dec 6, 2017

Member

1. This is a false test.

At a minimum we need to blow away the local repo cache in-between testing fileAt otherwise we will miss the fetch failure from the revision, e.g. be86b317fb6c2c8572f14f49422ba92627f835b4 being treated as a branch ref spec, i.e. refs/heads/be86b317fb6c2c8572f14f49422ba92627f835b4 with its resulting log message of

git fetch origin +refs/heads/be86b317fb6c2c8572f14f49422ba92627f835b4:refs/origin/be86b317fb6c2c8572f14f49422ba92627f835b4
fatal: Couldn't find remote ref refs/heads/be86b317fb6c2c8572f14f49422ba92627f835b4

Because this test has already checked fileAt("dev",...) when dev was v3 the local cache will already contain e.g. be86b317fb6c2c8572f14f49422ba92627f835b4

2. This is an incomplete test

We do not cover the case where the v3 revision is in the remote repository but is not advertised on any head. For that case we would need to

sampleRepo.git("reset", "--hard", "HEAD^");
sampleRepo.write("file", "v4");
sampleRepo.git("commit", "--all", "--message=v4"); // dev

And, most critically, this is the test case that you have actually almost fixed (it's not fixed because we will also fail on fetch:

git fetch origin +refs/heads/be86b317fb6c2c8572f14f49422ba92627f835b4:refs/origin/be86b317fb6c2c8572f14f49422ba92627f835b4
fatal: Couldn't find remote ref refs/heads/be86b317fb6c2c8572f14f49422ba92627f835b4

But the change you have made is for the case where the revision is in the remote repository but not advertised on a branch.

@@ -819,7 +819,7 @@ public SCMRevision run(GitClient client, String remoteName) throws IOException,
if (branches.isEmpty()) {

This comment has been minimized.

Copy link
@stephenc

stephenc Dec 6, 2017

Member

The actual bug fix is the line above (need to change false to true)

List<Branch> branches = client.getBranchesContaining(hash, true);

paired with some deconstruction of the remote branch name back into the actual corresponding branch name two lines after your change...

// something like this... but will need debugging to confirm
String name = branches.stream()
    .map(getName)
    .filter(n->n.startsWith(Constants.R_REFS+context.remoteName()+"/"))
    .map(n->n.substring(Constants.R_REFS.length()+context.remoteName().length()+1))
    .first();

The change you have proposed would address the case where the revision is in the repository but not visible from refs/heads/ (could be visible from some other custom ref or could just not have been GC'd yet) but in that case because the refspec will be constructed as refs/heads/${revision} which does not exist, the fetch will fail on checkout of a shared library unless the workspace already has fetched the revision and the SCMFileSystem will fail unless the local repository cache already contains the revision.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Dec 6, 2017

Did not spend time following all the details, but sounds like you are handling this somehow.

@jglick jglick closed this Dec 6, 2017

@jglick jglick deleted the jglick:retrieveRevision branch Dec 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.