6593 improve usability of deploy new script#7606
Conversation
227e99c to
a1411a6
Compare
…com:gchq/sleeper into 6593-improve-usability-of-deploy-new-script
ca61688
added
the
needs-reviewer
| } | ||
|
|
||
| public void update() throws IOException, InterruptedException { | ||
| SleeperInstanceConfiguration deployConfig = SleeperInstanceConfiguration.builder().instanceProperties(properties).tableProperties(tablePropertiesList).build(); |
There was a problem hiding this comment.
This SleeperInstanceConfiguration is built twice, here and in the DeployInstanceRequest. Maybe it would be better to reference the instance and table properties fields directly when we write them to the local directory?
| deployConfig.getInstanceProperties(), | ||
| deployConfig.getTableProperties().stream()); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); |
There was a problem hiding this comment.
This method declares throws IOException, so it looks like the try/catch isn't needed.
| private final CdkCommand cdkCommand; | ||
| private final SleeperInternalCdkApp cdkApp; | ||
| private Path propertiesFile; | ||
| private Path configDir; |
There was a problem hiding this comment.
These fields can be declared final. See our coding conventions: https://github.com/gchq/sleeper/blob/develop/docs/development/conventions.md#use-of-final
| cdkCommand = Objects.requireNonNull(builder.cdkCommand, "cdkCommand must not be null"); | ||
| cdkApp = Objects.requireNonNull(builder.cdkApp, "cdkApp must not be null"); | ||
| propertiesFile = builder.propertiesFile; | ||
| configDir = builder.configDir; |
There was a problem hiding this comment.
Maybe we should validate that one or the other of these fields are set here?
|
|
||
| public Path getConfigDir() { | ||
| return configDir; | ||
| } |
There was a problem hiding this comment.
We try to avoid needing null checks on values retrieved from a getter. See our coding conventions: https://github.com/gchq/sleeper/blob/develop/docs/development/conventions.md#null-handling
Looking at how these are used in DeployInstance, I don't think think the new fields are needed at all. We can set this on the CdkCommand instead?
| instanceProperties.set(SUBNETS, "someSubnets"); | ||
| assertThat(instanceProperties) | ||
| .isEqualTo(SleeperInstanceConfiguration.fromLocalConfiguration( | ||
| instancePropertiesFile).getInstanceProperties()); |
There was a problem hiding this comment.
I don't think this is the correct behaviour? We don't want to edit the user's configuration file, so we need to pass these as CDK context variables. I don't understand what's doing this in the production code, it looks like the code that was writing the file before is faked for the test.
| void shouldDeployNewInstanceWhenUsingInstanceProperties() throws Exception { | ||
| //When | ||
| deployNewInstance("scriptsDir", "someInstance", "someVpc", "someSubnets", "--instance-properties", | ||
| instancePropertiesFile.toString()); |
There was a problem hiding this comment.
Please can we avoid passing the scripts directory explicitly as part of the arguments? The user won't set that.
It also looks like this is not the correct value for that? Why isn't that causing a problem?
There was a problem hiding this comment.
I've made deployNewInstance add "scriptsDir" itself so the tests dont pass it in. The scripts directory is only used in the main method when creating a new DeployInstance which my tests don't interact with as I'm passing in the way to record the requests instead of actually deploying anything.
| ClientUtils.clearDirectory(configDir); | ||
| SaveLocalProperties.saveToDirectory(configDir, | ||
| deployConfig.getInstanceProperties(), | ||
| deployConfig.getTableProperties().stream()); |
There was a problem hiding this comment.
This code is duplicated in a couple of places, can we put it in SaveLocalProperties?
| deployPaused = builder.deployPaused; | ||
| public DeployNewInstance(InstanceDeployer deployInstance, | ||
| StoreFactory storeFactory, | ||
| SleeperInstanceConfiguration deployInstanceConfiguration, |
There was a problem hiding this comment.
I'm struggling to understand how this is passed around now, particularly as the path it comes from is passed as separate parameters alongside it.
| void setUp() throws IOException { | ||
| createTempDirectory(tempDir, null); | ||
| instancePropertiesFile = tempDir.resolve("instance.properties"); | ||
| Files.writeString(instancePropertiesFile, instanceProperties.saveAsString()); |
There was a problem hiding this comment.
This is generating an InstanceProperties object with InstancePropertiesTestHelper, and that simulates a pre-deployed instance. It's then writing it to a local file and treating it as a user's configuration file. I don't think it makes sense to treat one as the other, and I'm struggling to follow how these values are used.
patchwork01
removed
the
needs-reviewer
…le or confgDir being set
…lProperties to create directories
2 participants
Make sure you have checked all steps below.
Issue
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
Tests
Documentation
separate issue for that below.