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-52123] Adjust sse-gateway to the now configurable location for tasks logging #25

Merged
merged 2 commits into from Jun 26, 2018

Conversation

5 participants
@batmat
Copy link
Member

commented Jun 21, 2018

Since 2.114, logs location is configurable, and so logs can be elsewhere than /logs.
Tested manually.

Description

See JENKINS-52123.

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given
[JENKINS-52123] Adjust sse-gateway to the now configurable location f…
…or tasks logging

Since 2.114, logs location is configurable, and so logs can be elsewhere
than <rootDir>/logs.
@batmat

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

Gentle ping @tfennelly @rtyler who merged the last PRs.
cc @reviewbybees

@tfennelly
Copy link
Member

left a comment

LGTM thanks @batmat

jenkinsLogsDir = new File(overriddenLogsRoot);
}
return new File(jenkinsLogsDir, "sse-events");
}

This comment has been minimized.

Copy link
@tfennelly

tfennelly Jun 22, 2018

Member

Or maybe more simply ....

private File getHistoryDirectory(Jenkins jenkins) {
    final String overriddenLogsRoot = System.getProperty("hudson.triggers.SafeTimerTask.logsTargetDir");
    if (overriddenLogsRoot != null) {
        return new File(overriddenLogsRoot, "sse-events");
    } else {
        return new File(jenkins.getRootDir(), "/logs/sse-events");
    }
}

This comment has been minimized.

Copy link
@batmat

batmat Jun 22, 2018

Author Member

heh, 🤦‍♂ yeah. I guess it was late :P. Simpler, clearer, and much closer and consistent with what I did on the support-core plugin :) jenkinsci/support-core-plugin#140

Fixing. Thanks!

Make code more straightforward
Thanks for the review @tfennelly
@batmat

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2018

@tfennelly I addressed your comment. If you still approve it, can you possibly merge and release? Thanks a lot!

@batmat

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

Thanks @tfennelly !

As per the permissions file currently,

developers:
- "michaelneale"
- "vivek"
- "tfennelly"
- "kzantow"
- "tscherler"
- "jamesdumay"

@michaelneale @vivek @kzantow @scherler could someone please consider merging and releasing this plugin? Thanks a bunch!

@vivek

vivek approved these changes Jun 26, 2018

Copy link
Contributor

left a comment

LGTM

@vivek vivek merged commit 91db84c into jenkinsci:master Jun 26, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@vivek

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

@tfennelly Last release was more than year back. There have been many commits since then. Is master in shape we can release, is there extra testing needed before cutting a release?

* @return the root directory for logs.
*/
private File getHistoryDirectory(Jenkins jenkins) {
final String overriddenLogsRoot = System.getProperty("hudson.triggers.SafeTimerTask.logsTargetDir");

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jun 27, 2018

Member

Seems unnecessarily tied to an implementation detail of core.

This comment has been minimized.

Copy link
@batmat

batmat Jun 27, 2018

Author Member

@daniel-beck what do you mean? That those files should be put elsewhere?

I think the core should actually maybe even more define an API for those things to avoid putting random files everywhere BTW. Cf. also the ongoing initiative about Jenkins logging in general.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jun 27, 2018

Member

what do you mean?

You don't check a core API to make this distinction, but the mechanism by which core (currently) decides to put those files elsewhere.

@batmat batmat deleted the batmat:JENKINS-52123 branch Jun 27, 2018

@batmat

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

@vivek BTW, I suppose we could consider an alpha or beta release, i.e. into the experimental Update Center? That would help us move forward on Jenkins Essentials side, and would also make it easier (for us included) to test the release does not seem to break anything?

WDYT?

@vivek

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

@batmat I think some testing with blueocean will be good to have before beta or final release. It will also help me to understand motivations for release and how it ties up or blocks essentials?

@batmat

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2018

We aggressively changed the Jenkins data location for Essentials. Mainly to simplify JEP 302, but also in general to provide an opinionated version of Jenkins that separates more "static" and varying data (config vs. builds for instance, or logs in the current matter). So we customized Jenkins core so that logs could now be sent elsewhere than <Jenkins.rootDir>/logs, i.e. under JENKINS_HOME where we try to put only static or config related data (no logs, no builds, no exploded binaries, etc.).

As it is now, sse-gateway was making one our tests fail, so we disabled it while the current PR is in the release pipeline:

https://github.com/jenkins-infra/evergreen/blob/92f866a76691b685a8b1c751a09d1bde7d53fb69/distribution/tests/tests.sh#L91-L97

So, this PR is needed to avoid the sse-gateway plugin to put variable data under JENKINS_HOME, which could impede our ability to implement the snapshotting system (aka JEP 302) soon.

Hope this clarifies. If needed, we can also complete this picture by having a call together @vivek, just let me know.

@vivek vivek referenced this pull request Jul 3, 2018

Closed

Latest sse-gateway plugin #1767

0 of 7 tasks complete
@vivek

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

@batmat Thanks for providing context. I have opened a PR with sse-gateway plugin jenkinsci/blueocean-plugin#1767. It will be good to have this PR manually tested, feel free to contribute, we also are going to test it. Once all looks good we can cut release.

@batmat

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

I think as soon as https://issues.jenkins-ci.org/browse/INFRA-1700 is fixed, I will file a PR to incrementalify blueocean so that we can easily test it in Essentials (or anywhere else). cc @rtyler @mandie722

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

@vivek Hi! Any release ETA?

@vivek

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

@oleg-nenashev We verified on blueocean side that it works well. However, this plugin still issues with running cleanly in PCT, that is it fails PCT tests. Something we should resolve before releasing?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

@vivek sorry, I do not know what is the release process for this plugin

@batmat

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

@vivek can you point us to some public places to the PCT failure you're referring to? Or have someone knowing about that commenting here?

Having a release would be great to have the changes in master out. As we all know, the more we wait the riskier it gets as changes are accumulating (fortunately this plugin is still quite small).

We are actually seeing obsolete warnings coming into Jenkins Evergreen backend for code that is already fixed/changed in master.

Thanks!

@batmat

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

I would say, if this works in Blue Ocean, this is the most important aspect. We should release it, possibly in alpha to have it only in experimental temporarily, and see what needs to be fixed for PCT to succeed.

@batmat

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

FTR picked-up finally in jenkinsci/blueocean-plugin#1809

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.