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-37694] Display artifacts of a running build #5496

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

jglick
Copy link
Member

@jglick jglick commented May 17, 2021

See JENKINS-37694.

Proposed changelog entries

  • The build index page will now display an artifact summary and link to the artifacts list even if the build is still running.

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

building=\
The build is still in progress. \
Some artifacts are present but the list may be incomplete. \
Artifacts still being written may even be corrupt.
Copy link
Member Author

Choose a reason for hiding this comment

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

File corruption could potentially be avoided if

File f = new File(baseDir, te.getName());
if (!f.toPath().normalize().startsWith(baseDir.toPath())) {
throw new IOException(
"Tar " + name + " contains illegal file name that breaks out of the target directory: " + te.getName());
}
if (te.isDirectory()) {
mkdirs(f);
} else {
File parent = f.getParentFile();
if (parent != null) mkdirs(parent);
writing(f);
if (te.isSymbolicLink()) {
new FilePath(f).symlinkTo(te.getLinkName(), TaskListener.NULL);
} else {
IOUtils.copy(t, f);
were reworked to use a temporary file + atomic move, but that could have farther-ranging effects. Anyway we already rendered the artifact list from direct navigation, exposed artifacts via REST, etc. for running builds and no one seems to have complained about this not being atomic.

Copy link
Member

Choose a reason for hiding this comment

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

seems a bit weird to include the last line in a user facing message =/.

I would omit it

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it could potentially happen to you that the build happens to be in the middle of archiving some 3Gb binary, and you see it listed on the artifacts page and innocently go to download it and get a partial file. Rather unlikely in practice but seems appropriate to note the possibility.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Would it make sense to just fix the issue and prevent corruption?

@jglick
Copy link
Member Author

jglick commented May 24, 2021

just […] prevent corruption

As in #5496 (comment) I think the risk involved in changing how the general tar function works, especially verifying that this works properly on Windows and NFS (how many times have we patched the likes of AtomicFileWriter?), probably outweighs the small risk of partial artifacts being listed & served in more cases (they already were in some cases). I would rather not hold up this small and low-risk fix for a very real and commonly encountered bug.

Also note that users of artifact-manager-s3 (or any other plausible JEP-202 implementation using third-party blob storage) should not have any such issue to begin with.

@res0nance res0nance added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Jun 20, 2021
@res0nance res0nance merged commit e58f9d9 into jenkinsci:master Jun 22, 2021
@jglick jglick deleted the artifacts-JENKINS-37694 branch June 22, 2021 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants