-
Notifications
You must be signed in to change notification settings - Fork 202
Added support to choose configMap name #1636
Conversation
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
public class ConfigMap { | ||
|
||
@Parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that is required.
strange, looking at code I think it should work. I would pull your branch and test on my machine. |
Oops, a test is failing. Maybe that can give us an idea if we're missing something. |
Strange.. I will check here..
Thanks
|
The error is in
method ConfigMapEnricher.getConfigMapFromXmlConfiguration(), this method is
not loading the configs for ConfigMap in tests scope.
I've made a change that will not throws a NPE, the tests will pass now. :)
|
Codecov Report
@@ Coverage Diff @@
## master #1636 +/- ##
============================================
- Coverage 34.81% 34.77% -0.04%
Complexity 1121 1121
============================================
Files 186 186
Lines 10309 10314 +5
Branches 1679 1680 +1
============================================
- Hits 3589 3587 -2
- Misses 6299 6305 +6
- Partials 421 422 +1 |
@h3nrique : Works for me for this configuration:
What configuration were you trying for which it wasn't working? Could you please share? |
@rohanKanojia I've identified that when uses variable (ex: ${project.artifactId}) the configmap It's not created. I'm working in this here. |
@lordofthejars : @paoloantinori is trying to create configmap using this configuration:
but it doesn't seem to be getting picked up. Could you please take a look into this if you have time? Is this configuration supported? |
enricher/standard/src/main/java/io/fabric8/maven/enricher/standard/ConfigMapEnricher.java
Show resolved
Hide resolved
@h3nrique : Is it working now for you? for your |
Now It's working as well, I made a update that you say.
Thanks.
|
@h3nrique : cool, can you please mark this draft ready for merge? I think we can merge it then 👍 |
@h3nrique : Sorry, I forgot to tell you. Could you please add a line to CHANGELOG.md regarding this? |
Great this is exactly what I need |
Fixes #1634