Halyard separation #789
Halyard separation #789
Conversation
| return FileUtil.readAsString(new File(value)); | ||
| } catch (IOException e) { | ||
| throw new HalException(Problem.Severity.FATAL, | ||
| "Config references file that is unreadable: " + value); |
There was a problem hiding this comment.
I don't think this should read "config", instead "Was passed parameter " + value + " to unreadable file: " + e.getMessage()
| @@ -140,6 +155,8 @@ | |||
| builder.setValidate(doValidate); | |||
| builder.setRevert(() -> halconfigParser.undoChanges()); | |||
| builder.setSave(() -> halconfigParser.saveConfig()); | |||
| Path stagingPath = halconfigDirectoryStructure.getConfigPath(deploymentName); | |||
There was a problem hiding this comment.
Should this be named "configPath"? (Here and in a few other spots).
There was a problem hiding this comment.
If by a few, you mean 30. Yes ;)
| @Data | ||
| public class GlobalApplicationOptions { | ||
|
|
||
| private static final String CONFIG_PATH = "/tmp/hal.config"; |
There was a problem hiding this comment.
I think tmp is fair game to be deleted
There was a problem hiding this comment.
I think it's better to keep this close to other halyard config, so something like /opt/halyard/config or ~/.hal
| f.setAccessible(true); | ||
| return (String) f.get(n); | ||
| } catch (IllegalAccessException e) { | ||
| throw new HalException(FATAL, "Failed to clean staging directory: " + e.getMessage(), e); |
There was a problem hiding this comment.
This feels more like a runtime error than a halexception - by convention the latter is reserved for errors the user can do something about.
| * Deletes all files in the staging directory that are not referenced in the hal config. | ||
| */ | ||
| public void cleanLocalFiles(Path stagingDirectoryPath) { | ||
| if (GlobalApplicationOptions.getInstance().isUseRemoteDaemon()) { |
There was a problem hiding this comment.
This needs to be configured another way - those config options are used by the CLI - this should use spring.
| * Deletes all files in the staging directory that are not referenced in the hal config. | ||
| */ | ||
| public void cleanLocalFiles(Path stagingDirectoryPath) { | ||
| if (globalApplicationOptions.isUseRemoteDaemon()) { |
There was a problem hiding this comment.
Suggest returning early:
if (!globalApplicationOptions.isRemoteDaemon()) {
return
}
| @@ -169,6 +183,30 @@ public void parentify() { | |||
| return res; | |||
| } | |||
|
|
|||
| public void stageLocalFiles(Path outputPath) { | |||
| if (GlobalApplicationOptions.getInstance().isUseRemoteDaemon()) { | |||
| @PropertySource("file:" + GlobalApplicationOptions.CONFIG_PATH) | ||
| public class GlobalApplicationOptions { | ||
|
|
||
| static final String CONFIG_PATH = "/opt/spinnaker/hal.properties"; |
There was a problem hiding this comment.
Is this config shared between the client and the server? Based on the javadoc string, it appears so.
All of the other components follow a convention for server configuration options that hooks into Spring's parsing and options logic (here's a guide on how that works: https://www.travistomsu.com/2016/12/19/configuring-spinnaker/)
If possible, I'd suggest using the yaml format instead of .properties files.
There was a problem hiding this comment.
The reason this is done this way is because the CLI doesn't use spring and i'd prefer to share the implementation between the two.
There was a problem hiding this comment.
Also its still Autowire-ed in the halyard-web backend.
Halyard can now be configured to deploy referenced files by value instead of by filesystem reference.