Skip to content

Run setIndex when revisionState==null #75

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

Open
haiyanghaiyang wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Run setIndex when revisionState==null #75

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/main/java/hudson/plugins/repo/ManifestAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

@francoisferrand francoisferrand Nov 8, 2021 •
edited
Loading

Copy link
Copy Markdown

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 ?)

@haiyanghaiyang haiyanghaiyang Nov 8, 2021

Copy link
Copy Markdown
Author

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.

@TheStrix TheStrix Dec 15, 2021

Copy link
Copy Markdown

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).

@francoisferrand francoisferrand Dec 22, 2021

Copy link
Copy Markdown

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.

}

return revisionState == null ? "" : revisionState.getManifest();
}

/**
* Returns the manifest repository's url for this repo snapshot.
*/
public String getUrl() {
if (revisionState == null) {
setIndex(index);
}

return revisionState == null ? "" : revisionState.getUrl();
}

/**
* Returns the manifest repository's branch name for this repo snapshot.
*/
public String getBranch() {
if (revisionState == null) {
setIndex(index);
}

return revisionState == null ? "" : revisionState.getBranch();
}

/**
* Returns the path to the manifest file for this repo snapshot.
*/
public String getFile() {
if (revisionState == null) {
setIndex(index);
}

return revisionState == null ? "" : revisionState.getFile();
}
}