Add a step for reading properties into the environment. #24
Conversation
Yes, this is admittedly kind of a shortcut for a combination of readProperties and withEnv, but hey, no harm in simplification. Also moved to newer versions of various dependencies because I wanted to use modern stuff and style.
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>plugin</artifactId> | ||
| <version>1.609.1</version> | ||
| <version>2.29</version> <!-- TODO: Move to 2.31 or later once https://youtrack.jetbrains.com/issue/IDEA-175538 is resolved --> |
There was a problem hiding this comment.
please make pom version bumps in a separate PR, since there are more to it than just bumping the versions.
For example in the same swoop I wan't to get rid of all the @Inject et.al. stuff. And have this plugin be as much of a model example of how to do pipeline steps as possible.
There was a problem hiding this comment.
nod. I'll do that first and redo this on top of it.
| static Map<String,String> toStringMap(@Nonnull Map<String,Object> inMap) { | ||
| Map<String,String> result = new TreeMap<>(); | ||
| for (Map.Entry<String,Object> e : inMap.entrySet()) { | ||
| result.put(e.getKey(), e.getValue().toString()); |
There was a problem hiding this comment.
e.getValue() could return null. Use String.valueOf(e.getValue()) instead, unless that returns the string "null":)
|
|
||
| if (!StringUtils.isBlank(step.getText())) { | ||
| result.putAll(ConfReaderUtils.toStringMap(ConfReaderUtils.readProperties(step.getText(), | ||
| step.getDefaults()))); |
There was a problem hiding this comment.
The "defaults" here should be the result of reading the file iirc.
There was a problem hiding this comment.
Otherwise you'll risk overwriting what was read from disk with what was specified as defaults
| public static class DescriptorImpl extends StepDescriptor { | ||
| @Override | ||
| public String getFunctionName() { | ||
| return "propertiesWrapper"; // TODO: Probably a better name is needed |
| public abstract class AbstractPropertiesStep extends Step { | ||
| public abstract String getFile(); | ||
|
|
||
| public abstract void setFile(String file); |
There was a problem hiding this comment.
why having these 'abstract'? Would be less code duplication in the other classes if the fields and annotations where in this class instead.
| } | ||
| for (Map.Entry<Object, Object> entry : properties.entrySet()) { | ||
| result.put(entry.getKey() != null ? entry.getKey().toString(): null, entry.getValue()); | ||
| result.putAll(ConfReaderUtils.readProperties(step.getText(), step.getDefaults())); |
| if (f.exists() && !f.isDirectory()) { | ||
| properties.load(f.read()); | ||
| } else if (f.isDirectory()) { | ||
| logger.print("warning: "); |
There was a problem hiding this comment.
is it really just a warning? listener.error() ?
|
@abayer do you plan to have another look? That would be awesome, thanks! |
|
@abayer I tried bumping the same dependencies to use some of the newer things but started fighting the upper bound requirements and got stuck unable to run unit tests. 😭 |
|
@abayer this is conflicted FYI. |
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
Is this really necessary? Seems like something you can already write for yourself without a lot of hassle.
|
@jglick well I can show you the code we wrote for that, maybe this can be simplified, but right now I think this is a bit too complicated for what it does. Also Declarative is IMO about making things work as much OOTB as possible nicely and concisely in an expressive way. And that feature would look like definitely contributing to this. |
6 participants
Yes, this is admittedly kind of a shortcut for a combination of
readProperties and withEnv, but hey, no harm in simplification.
Also moved to newer versions of various dependencies because I wanted
to use modern stuff and style.
cc @reviewbybees