Skip to content

Conversation

@ringerc
Copy link
Contributor

@ringerc ringerc commented Nov 8, 2018

See JENKINS-54532.

Proposed changelog entries

  • Document how to get build parameters from a Run

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

@oleg-nenashev
Copy link
Member

I will poke the build once the PR builder is fixed

@oleg-nenashev oleg-nenashev self-requested a review December 20, 2018 11:30
@oleg-nenashev oleg-nenashev self-assigned this Dec 20, 2018
@ringerc
Copy link
Contributor Author

ringerc commented Dec 21, 2018

Thanks for the fixes, sorry this was a bit fire-and-forget.

@oleg-nenashev
Copy link
Member

Neither blueocean nor common UI show what is the root cause easily.
@abayer @dwnusbaum would appreciate some Pipeline advice

@dwnusbaum
Copy link
Member

dwnusbaum commented Jan 31, 2019

From the logs it looks like a test failed? Not sure why it didn't show up in the test view in Blue Ocean or classic UI:

03:24:32 [ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 13.48 s <<< FAILURE! - in jenkins.model.StartupTest
03:24:32 [ERROR] noWarnings(jenkins.model.StartupTest)  Time elapsed: 9.746 s  <<< FAILURE!
03:24:32 java.lang.AssertionError: expected:<[]> but was:<[Running Jenkins with Java 11 which is available in the preview mode only. A custom experimental update center will be used: https://updates.jenkins.io/temporary-experimental-java11/]>
03:24:32 	at org.junit.Assert.fail(Assert.java:88)
03:24:32 	at org.junit.Assert.failNotEquals(Assert.java:834)
03:24:32 	at org.junit.Assert.assertEquals(Assert.java:118)
03:24:32 	at org.junit.Assert.assertEquals(Assert.java:144)
03:24:32 	at jenkins.model.StartupTest.noWarnings(StartupTest.java:49)
03:24:32 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
03:24:32 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
03:24:32 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
03:24:32 	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
03:24:32 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
03:24:32 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
03:24:32 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
03:24:32 	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
03:24:32 	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:549)
03:24:32 	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
03:24:32 	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
03:24:32 	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
03:24:32 	at java.base/java.lang.Thread.run(Thread.java:834)

@oleg-nenashev
Copy link
Member

From the logs it looks like a test failed? Not sure why it didn't show up in the test view in Blue Ocean or classic UI:

@dwnusbaum It is a separate issue, see #3811 by @jglick . It does not impact the build, because the test is retried after that. We will cleanup up this misleading error in few week once Java 11 is in GA

@dwnusbaum
Copy link
Member

dwnusbaum commented Feb 1, 2019

The build failed in the Windows JDK 8 branch because of a Javadoc error:

C:\Jenkins\workspace\Core_jenkins_PR-3724\core\src\main\java\hudson\model\Run.java:147: error: reference not found
 * accessible via {@link Action#getAllActions} and {@link #getAction(Class)}. 

Looks like a visualization bug in BlueOcean. If you click on the Windows JDK 8 branch then it does show the failing step at the end, so I'm not sure what happened. I looked through Jira but didn't find a similar bug already reported, so maybe it's a regression from some other fix?

@oleg-nenashev
Copy link
Member

Thanks @dwnusbaum !

@daniel-beck daniel-beck self-assigned this Apr 9, 2019
* It will use a gzip-compressed log file (log.gz) if that exists.
*
* It will use a gzip-compressed log file (log.gz) if that exists, decompressing
* it on the fly to return the uncompressed data.
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of this being documented in the API (strictly speaking, this means JEP-210 is invalid), but at least this PR doesn't add it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah to complement #3963 I would suggest simply deleting this sentence. How or where the log text is stored is a matter of no concern to the caller of this method, and for Pipeline there is no built-in gzip support (this implementation is not used).

Copy link
Member

Choose a reason for hiding this comment

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

+1, also this would become wrong indeed if we ended up merging #3966

Copy link
Member

Choose a reason for hiding this comment

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

Removing it at all since there is a consensus. Not much related to the PR anyway

* It will use a gzip-compressed log file (log.gz) if that exists.
*
* It will use a gzip-compressed log file (log.gz) if that exists, decompressing
* it on the fly to return the uncompressed data.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah to complement #3963 I would suggest simply deleting this sentence. How or where the log text is stored is a matter of no concern to the caller of this method, and for Pipeline there is no built-in gzip support (this implementation is not used).

* Many details of a run are contained in {@link Action} instances
* accessible via {@link Actionable#getAllActions} and {@link #getAction(Class)}.
* For example, build access to build parameters is obtained with
* <code>run.getAction(hudson.model.ParametersAction.class)</code> to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* <code>run.getAction(hudson.model.ParametersAction.class)</code> to
* {@code run.getAction(ParametersAction.class)} to

}

/**
* Returns a Reader that reads from the log file.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Returns a Reader that reads from the log file.
* Reads from the log file.

Stating the return type of a method (and without @code or @link, at that) is not helpful.

/**
* Returns a Reader that reads from the log file.
*
* This is the simplest way to access the job's console log.
Copy link
Member

Choose a reason for hiding this comment

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

Well…not necessarily. Depends on what you wanted to accomplish. For example, this will contain raw console notes, which might be unwanted in some contexts. getLogText is the most general way to access the log, as you can use that to work with the log in a variety of ways.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, we should not be doing arguable suggestions, especially if we return to JEP-207

Suggested change
* This is the simplest way to access the job's console log.

@batmat batmat self-assigned this Apr 11, 2019
HedAurabesh added a commit to HedAurabesh/jenkins that referenced this pull request Apr 11, 2019
The previous code was missing a change to getLogFile(), that was
not applied due to the deprecation of getLogFile() in the upstream code.

This led to the new files never being read. The new code will pick up
compressed log files regardless of their extension, as long as they are
named "log" or "log.*" -- with the uncompressed version always being
preferred.

Also:
 - Extended the logging to give some additional feedback to the users
   in case of errors or missing files
 - Rephrased the API docs to no longer refer to log files
   (See: jenkinsci#3724)
@daniel-beck
Copy link
Member

@batmat Why is this assigned to you?

@batmat
Copy link
Member

batmat commented Apr 22, 2019

@daniel-beck no particular reason. IIRC was just a way to add it to my bucket and get it over the fence. But I failed since.
Please anyone feel free to move without me, and/or reassign to someone else.

* It will use a gzip-compressed log file (log.gz) if that exists.
*
* It will use a gzip-compressed log file (log.gz) if that exists, decompressing
* it on the fly to return the uncompressed data.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* it on the fly to return the uncompressed data.

* It will use a gzip-compressed log file (log.gz) if that exists.
*
* It will use a gzip-compressed log file (log.gz) if that exists, decompressing
* it on the fly to return the uncompressed data.
Copy link
Member

Choose a reason for hiding this comment

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

Removing it at all since there is a consensus. Not much related to the PR anyway

* Returns an input stream that reads from the log file.
* It will use a gzip-compressed log file (log.gz) if that exists.
*
* It will use a gzip-compressed log file (log.gz) if that exists, decompressing
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* It will use a gzip-compressed log file (log.gz) if that exists, decompressing

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.

I have no permission to push to the original repository, so I cannot apply the proposed changes.
@ringerc if you are still interested to get it over the line, please apply changes so that we can integrate it

@rsandell rsandell added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Aug 1, 2019
@batmat batmat added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Sep 19, 2019
@batmat
Copy link
Member

batmat commented Sep 19, 2019

Given no answer since May, we will close this Pull Request on next pass.

Thanks a lot for your understanding.

@fcojfernandez
Copy link
Member

Closing since there is no answer since May and it's proposed for close. Please reopen the PR if you plan keep on working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants