gchq / gchq/sleeper

Can't save instance properties in the admin client if any table has invalid configuration

Open
#6,768 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Java
Stars
107
Forks
29
Avg merge
19h 46m
Merged PRs (30d)
141

Description

### Description / Background

When the admin client updates the instance properties, it saves the table properties to a local directory so that they're available to the CDK when the instance needs to be deployed to apply the change.

If any table has invalid configuration, this throws an exception, and you can't save the instance properties.

It also writes the properties to a local folder whether or not it needs to invoke the CDK, which is not necessary.

### Steps to reproduce

1. Create a table
2. Set its schema to "{}" in its table properties in the config bucket
3. Open the admin client and edit any instance property
4. See error

### Expected behaviour

The instance properties should be saved.

An invalid table should not be passed to the CDK.

### Technical Notes / Implementation Details

This happens in AdminClientPropertiesStore.saveInstanceProperties.

This could do with re-testing as we've recently changed TablePropertiesStore.streamAllTables to remove validation when loading many tables at once. That seems to be where this was failing when we originally raised this issue. We can pay attention to what happens in the CDK when it receives the invalid table properties as well, and see if there's anything that could go wrong there with the current behaviour. We could consider adding logic to exclude invalid tables from the CDK deployment.

If everything works now, the main thing is to add test coverage. We'll want to cover this case in AdminClientPropertiesStoreIT.

If any changes are needed in SaveLocalProperties we can add test coverage in SaveLocalPropertiesIT.

We could also think about, and possibly test, the logic for when to write the properties to the generated directory. The only reason that the table properties are written to the generated directory is because the CDK uses them in DashboardStack to create a dashboard for each Sleeper table. Right now they're written to the generated directory whether or not we invoke the CDK. That might be fine, because otherwise an old version of the properties can get left in the generated directory, and if you saw them there you might assume they're up to date. That seems to mean there's nothing to do on this issue for that, unless we can think of tests to add that aren't already there.

Contributor guide

Open the contributing guide

Research direction

Start in AdminClientPropertiesStore.saveInstanceProperties and reproduce the case with a table schema of "{}". Run AdminClientPropertiesStoreIT, then inspect SaveLocalPropertiesIT if changes there are needed. Done means instance properties save successfully, invalid tables are not passed to the CDK, and relevant test coverage exists.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend, infrastructure, testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.