feat(#937): Overrides environment variables when defined in resources

[lordofthejars]

Fix #937 Still no tests or applied to all enrichers because I am not sure about the approach. I think this is the cleanest way of doing it, but waiting for some feedback.

[devang-gaur]

Hey @lordofthejars , this_underscore_format isn't consistent with the codebase. I know the the test method name is really long and will look clumsy in camelCase format but i think we should prioritize consistency of the code format here.

As per the accordance with the current format it can be simply,

@Test
public void setEnvironmentVarFromResources(....) { }

If there's a test annotation already, 'should' would look redundant in indicating assertions.

My $0.02.

[codecov]

Codecov Report

Merging #1434 into master will increase coverage by 0.32%.
The diff coverage is 37.14%.

@@             Coverage Diff              @@
##             master    #1434      +/-   ##
============================================
+ Coverage     34.63%   34.96%   +0.32%     
- Complexity     1045     1052       +7     
============================================
  Files           172      171       -1     
  Lines          9553     9536      -17     
  Branches       1635     1620      -15     
============================================
+ Hits           3309     3334      +25     
+ Misses         5869     5839      -30     
+ Partials        375      363      -12

[rhuss]

Really not sure why we need to duplicate that piece of code in every enricher. For me that looks like a smell.

Whats about a post processing step outside of all enricher, which just overrides the environment variables added by the enricher chain ?

[lordofthejars]

well actually this comes of a conversation we had about allowing enrichers to choose if they want to use it or not. Missatge de Roland Huß <[email protected]> del dia dj., 13 de des. 2018 a les 14:43:

…

***@***.**** commented on this pull request. ------------------------------ In enricher/standard/src/main/java/io/fabric8/maven/enricher/standard/DependencyEnricher.java <#1434 (comment)> : > @@ -119,6 +120,12 @@ private void addArtifactsWithYaml(Set<URL> artifactSet, String dependencyYaml) { @OverRide public void adapt(final KubernetesListBuilder builder) { + + getConfiguration().getResource().ifPresent(resourceConfig -> { + overrideEnvironmentVariables(builder, resourceConfig.getEnv() + .orElse(new HashMap<>())); + }); + Really not sure why we need to duplicate that piece of code in every enricher. For me that looks like a smell. Whats about a post processing step outside of all enricher, which just overrides the environment variables added by the enricher chain ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1434 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABcmYddBB-BaD32CDp5BSYPnTMFONNDmks5u4lltgaJpZM4YdfqU> .

-- +----------------------------------------------------------+ Alex Soto Bueno - Computer Engineer www.lordofthejars.com +----------------------------------------------------------+

[rhuss]

@lordofthejars yeah, I remember darkly. Do you have an example for an enricher who does the environment differently ? Not sure anymore whether we should guarantee a consistent behaviour over all enrichers or not.

[rhuss]

Test are failing because of a missing JMockit dependency. @lordofthejars could you please have a look ?