-
Notifications
You must be signed in to change notification settings - Fork 91
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
Run setIndex when revisionState==null #75
base: master
Are you sure you want to change the base?
Conversation
The setIndex is not called when open existing job's manifest information. At that time, revisionState==null, and the manifest cannot be fetched. The repo manifest information is blank on web. The fix just calls setIndex when revisionState==null before get manifest.
e6d0784
to
e820940
Compare
@francoisferrand This PR looks great, could you merge it? 👍 |
This looks fishy IMO. The index is expected to be set via the Looking quickly at the code, maybe we are just missing a |
@@ -136,27 +136,43 @@ public final String getUrlName() { | |||
* Gets a String representation of the static manifest for this repo snapshot. | |||
*/ | |||
public String getManifest() { | |||
if (revisionState == null) { | |||
setIndex(index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if revisionState is null, then it means index was not set (so null
) : and this will just load the "first" manifest/banch/...
I think we need to get to bottom of this, i.e. understand why the index was not set properly via setIndex() (missing @databoundsetter
annotation maybe?) or why the revisionstate was not properly loaded at that time (timing between calls to setIndex and onLoad/onAttached ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you start a new build, then setIndex is called. However, when you open an existing job, and want to review saved manifest inside, the setIndex is not called. Then we'll get empty manifest information, i.e. blank content in UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, as @haiyanghaiyang above described, it's called on checkout() in RepoScm and index is set that first time, but when we queue some other build and review back the manifest for some build it returns "" for getManifest() since index is not set in ManifestAction (revisionState is null).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand (and fully accept) that setIndex()
is not called. However, my understand is that this PR will not fix the whole issue : it will just automatically load the first index, as if setIndex(0)
was called, and thus only deal with the case when there is a single manifest (which is indeed the most common).
A better fix would be to make sur the missing call to setIndex() happens when de-serializing ManifestAction objects, to that the correct manifest will be loaded/shown.
Issue solved by #82 |
The setIndex is not called when open existing job's
manifest information. At that time, revisionState==null,
and the manifest cannot be fetched. The repo manifest
information is blank on web.
The fix just calls setIndex when revisionState==null
before get manifest.