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

[WIP][JENKINS-26583] Env-inject suppress variables contributed by extension points #37

Merged
merged 4 commits into from
Oct 6, 2017

Conversation

olivergondza
Copy link
Member

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@olivergondza olivergondza changed the title [WIP][JENKINS-26583] Env-inject suppress variables contributed by extension points [JENKINS-26583] Env-inject suppress variables contributed by extension points Jan 24, 2015
@olivergondza olivergondza changed the title [JENKINS-26583] Env-inject suppress variables contributed by extension points [WIP][JENKINS-26583] Env-inject suppress variables contributed by extension points Jan 25, 2015

p.getBuildWrappersList().add(new ContributingWrapper("DISPLAY", "BUILD_VAL"));

validate(p);
Copy link
Member

Choose a reason for hiding this comment

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

please don't hide validation process in "mistic guest" this is antipattern of test writing. Use hamcrest matchers :) they cool

@olivergondza
Copy link
Member Author

@lanwen, I have addressed your concerns. Using hamcrest for validation* would require bumping the inherited version.

*) there are no Matcher#describeMismatch( or TypesafeMatcher#describeMismatchSafely) so it would actually made defect localization weaker.

@lanwen
Copy link
Member

lanwen commented May 5, 2015

@olivergondza thanks for quick reply!
Think junit version can be bumped to 4.12 without any doubt. This version is stable and used in production testing half a year. Test should use all features of new version of this lib and hamcrest lib

@oleg-nenashev
Copy link
Member

I would be also happy to see it merged

@olivergondza
Copy link
Member Author

The fix does not work is it should, I did not managed to untangle the injection logic IIRC.

@dummy-lanwen-bot
Copy link

SonarQube analysis reported 10 issues:

  • MAJOR 10 major

Watch the comments in this conversation to review them.
Note: the following issues could not be reported as comments because they are located on lines that are not displayed in this pull request:

@jukkasi
Copy link

jukkasi commented Jun 9, 2016

+1

We use EnvInject plugin and Delivery Pipeline plugin, which uses Build Name Setter Plugin which sets $BUILD_DISPLAY_NAME.

When $BUILD_DISPLAY_NAME is used in HipChat plugin message template, the value is not correct, uninstalling EnvInject plugin solves the problem.

@oleg-nenashev
Copy link
Member

Deleting garbage comments from @dummy-lanwen-bot

@oleg-nenashev oleg-nenashev added this to the envinject-2 milestone Sep 20, 2016
@oleg-nenashev
Copy link
Member

+1 for integrating it into EnvInject-2

@DanielRedOak
Copy link

I think we're seeing this problem in combination with the rvm plugin. GEM_PATH ends up with pre-rvm init values so it doesnt work. The job isnt using any envinject features, but it is still causing issues.

@oleg-nenashev oleg-nenashev self-assigned this Apr 27, 2017
@oleg-nenashev
Copy link
Member

I will check if I can integrate it towards the 2.1 release

@fiznool
Copy link

fiznool commented Jun 13, 2017

The 2.1 release came and went without this getting merged... any plans on getting this in to a future release? It's blocking my use of the nodejs plugin 😞

@oleg-nenashev
Copy link
Member

@fiznool There is a plan for it, but it is not trivial to fix it without breaking changes in other usa-cases. Right now I do not have so much spare time to work on EnvInject (the plugin is waiting for adoption), and effectively this change priority is being defined by the internal priorities of the company I work for. These priorities shift sometimes, hence I cannot commit on the ETA.

If somebody proposes a fix, I will be happy to review it.

@fiznool
Copy link

fiznool commented Jun 13, 2017 via email

@oleg-nenashev
Copy link
Member

Created #124, which really fixes some bits

@oleg-nenashev
Copy link
Member

Closing in favor of #125, which offers a partially working fix covering JENKINS-26583.
2 tests created by @olivergondza still fails, but I think they can be fixed later since the fix logic is quite different (see #124).

@oleg-nenashev oleg-nenashev merged commit 6f6a8dd into jenkinsci:master Oct 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants