Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle multiple backends for the TypeInstance #657

Merged
merged 13 commits into from
Mar 24, 2022
8 changes: 8 additions & 0 deletions hack/images/jinja2/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ postgres
Prefix for db was removed. This is Jinja limitation. It shouldn't be a big problem as long
as there is no need to render the template twice with the same prefix.

### Configuration

There is a possibility of pre-processing data by setting options in the configuration file.
List of supported operations:
| Name | Default | Description |
| ------------------------- | ------------ | ---------------------------------------------------------------------------------------------------|
| prefix | "" | Adds a prefix to inputted data. The data will be accessible using the set prefix. |
| unpackValue | False | If the `value` prefix is set in the inputted data then the data will be unpacked from that prefix. |

## Prerequisites

Expand Down
15 changes: 12 additions & 3 deletions hack/images/jinja2/jinja2-cli/jinja2cli/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -312,15 +312,13 @@ def cli(opts, args, config): # noqa: C901

out = codecs.getwriter("utf8")(out)

if config.get("prefix") is not None and len(parsed_data) != 0:
parsed_data = {config["prefix"]: parsed_data}
parsed_data = preprocessing_data(config, parsed_data)

template_path = os.path.abspath(template_path)
out.write(render(template_path, parsed_data, extensions, opts.filters, opts.strict))
out.flush()
return 0


def parse_kv_string(pairs):
dict_ = {}
for pair in pairs:
Expand All @@ -329,6 +327,17 @@ def parse_kv_string(pairs):
return dict_


def preprocessing_data(config, data):
'''Return preprocessed data based on the applied configuration.'''

if config.get("unpackValue") is True and len(data) != 0 and "value" in data:
data = data.get("value", {})

if config.get("prefix") is not None and len(data) != 0:
data = {config["prefix"]: data}

return data

class LazyHelpOption(Option):
"An Option class that resolves help from a callable"

Expand Down
59 changes: 46 additions & 13 deletions hack/images/jinja2/jinja2-cli/tests/test_jinja2cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,76 +12,82 @@


@dataclass
class TestCase:
class RenderTestCase:
name: str
template: str
data: typing.Dict[str, typing.Any]
result: str

@dataclass
class PreprocessingDataTestCase:
name: str
config: typing.Dict[str, typing.Any]
data: typing.Dict[str, typing.Any]
result: typing.Dict[str, typing.Any]

render_testcases = [
TestCase(name="empty", template="", data={}, result=""),
TestCase(
RenderTestCase(name="empty", template="", data={}, result=""),
RenderTestCase(
name="simple",
template="<@ title @>",
data={"title": b"\xc3\xb8".decode("utf8")},
result=b"\xc3\xb8".decode("utf8"),
),
TestCase(
RenderTestCase(
name="prefix",
template="<@ input.key @>",
data={"input": {"key": "value"}},
result="value",
),
TestCase(
RenderTestCase(
name="two prefixes but one provided",
template="<@ input.key @>/<@ additionalinput.key @>",
data={"input": {"key": "value"}},
result="value/<@ additionalinput.key @>",
),
TestCase(
RenderTestCase(
name="missing prefix",
template="<@ input.key @>",
data={},
result="<@ input.key @>",
),
TestCase(
RenderTestCase(
name="items before attrs",
template="<@ input.values.key @>",
data={"input": {"values": {"key": "value"}}},
result="value",
),
TestCase(
RenderTestCase(
name="attrs still working",
template="<@ input.values() @>",
data={"input": {}},
result="dict_values([])",
),
TestCase(
RenderTestCase(
name="key with dot",
template="<@ input['foo.bar'] @>",
data={"input": {"foo.bar": "value"}},
result="value",
),
TestCase(
RenderTestCase(
name="missing key with dot",
template='<@ input["foo.bar"] @>',
data={},
result='<@ input["foo.bar"] @>',
),
TestCase(
RenderTestCase(
name="use default value",
template='<@ input["foo.bar"] | default("hello") @>',
data={},
result="hello",
),
TestCase(
RenderTestCase(
name="multiple dotted values",
template='<@ input.key.key["foo.bar/baz"] | default("hello") @>',
data={},
result="hello",
),
TestCase(
RenderTestCase(
name="multiline strings",
template="""<@ input.key.key["foo.bar/baz"] | default('hello
hello') @>""",
Expand All @@ -100,6 +106,33 @@ def test_render(tmp_path, case):
assert output == case.result


preprocessing_data_testcases = [
PreprocessingDataTestCase(
name="set prefix in the config should prefix the data",
config={"prefix": "testprefix"},
data = {"test": "test"},
result={"testprefix": {"test": "test"}}
),
PreprocessingDataTestCase(
name="set unpackValue in the config should remove the value prefix",
config={"unpackValue": True},
data = {"value": {"test": "test"}},
result={"test": "test"}
),
PreprocessingDataTestCase(
name="set unpackValue and prefix should output correct results",
config={"prefix": "testprefix", "unpackValue": True},
data = {"value": {"test": "test"}},
result={"testprefix": {"test": "test"}}
)
]

@pytest.mark.parametrize("case", preprocessing_data_testcases)
def test_preprocessing_data(case):
output = cli.preprocessing_data(case.config,case.data)
assert output == case.result


def test_random_password(tmp_path):
random_pass_path = tmp_path / "random.template"
random_pass_path.write_text("<@ random_password(length=4) @>")
Expand Down
3 changes: 1 addition & 2 deletions hack/images/merger/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@

This folder contains the Docker image which merges multiple input YAML files into a single one.

The Docker image contains the `merger.sh` helper script. The script is an entrypoint of the image, and it is used to prefix and merge all YAML files found in `$SRC` directory.
Each file is prefixed with a file name without extension.
The Docker image contains the `merger.sh` helper script. The script is an entrypoint of the image, and it is used to prefix and merge all YAML files found in `$SRC` directory. Additionally, if the YAML file contains the `value` key, then it is unpacked from that key. Each file is prefixed with a file name without extension.

## Installation

Expand Down
8 changes: 7 additions & 1 deletion hack/images/merger/merger.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,16 @@
SRC=${SRC:-"/yamls"}
OUT=${OUT:-"/merged.yaml"}

# prefix each file with its filename
for filename in "${SRC}"/*; do
filename=$(basename -- "$filename")
prefix="${filename%.*}"

# remove value key if exists
mkuziemko marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency reason, it would be good to also have unpackValue conf option here. When I went through our manifests, it was hard to understand why it works in this way, as this is a "hidden" functionality.

But let's do that on a new follow-up PR. It's not a blocker for merging this one 👍

if [[ $(yq e 'has("value")' "${SRC}"/"${filename}") == "true" ]]; then
yq e '.value' -i "${SRC}"/"${filename}"
fi

# prefix each file with its filename
yq e -i "{\"${prefix}\": . }" "${SRC}"/"${filename}"
done

Expand Down
4 changes: 2 additions & 2 deletions hack/lib/const.sh
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ readonly CAPACT_USE_TEST_SETUP="false"
#
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be removed before merge


readonly CAPACT_INCREASE_RESOURCE_LIMITS="true"
readonly CAPACT_HUB_MANIFESTS_SOURCE_REPO_URL="github.com/capactio/hub-manifests"
readonly CAPACT_HUB_MANIFESTS_SOURCE_REPO_URL="github.com/mkuziemko/hub-manifests"
# The git ref to checkout. It can point to a commit SHA, a branch name, or a tag.
# If you want to use your forked version, remember to update CAPACT_HUB_MANIFESTS_SOURCE_REPO_URL respectively.
readonly CAPACT_HUB_MANIFESTS_SOURCE_REPO_REF="main"
readonly CAPACT_HUB_MANIFESTS_SOURCE_REPO_REF="context_handling"
2 changes: 1 addition & 1 deletion hub-js/graphql/local/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ type TypeInstanceResourceVersionSpec {
abstract: backendRef.abstract,
fetchInput: {
typeInstance: { resourceVersion: rev.resourceVersion, id: ti.id },
backend: { context: backendCtx.context, id: backendRef.id}
backend: { context: apoc.convert.fromJsonMap(backendCtx.context), id: backendRef.id}
}
} AS value
RETURN value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export async function deleteTypeInstance(

// NOTE: Need to be preserved with 'WITH' statement, otherwise we won't be able
// to access node's properties after 'DETACH DELETE' statement.
WITH *, {id: ti.id, backend: { id: backendRef.id, context: specBackend.context, abstract: backendRef.abstract}} as out
WITH *, {id: ti.id, backend: { id: backendRef.id, context: apoc.convert.fromJsonMap(specBackend.context), abstract: backendRef.abstract}} as out
DETACH DELETE ti, metadata, spec, tirs, specBackend

WITH *
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export async function getTypeInstanceStoredExternally(

WITH {
typeInstanceId: ti.id,
backend: { context: backendCtx.context, id: backendRef.id, abstract: backendRef.abstract}
backend: { context: apoc.convert.fromJsonMap(backendCtx.context), id: backendRef.id, abstract: backendRef.abstract}
} AS value
RETURN value
`,
Expand Down
51 changes: 2 additions & 49 deletions hub-js/src/local/storage/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,6 @@ import {
import { JSONSchemaType } from "ajv/lib/types/json-schema";
import { TextEncoder } from "util";

// TODO(https://github.com/capactio/capact/issues/634):
// Represents the fake storage backend URL that should be ignored
// as the backend server is not deployed.
// It should be removed after a real backend is used in `test/e2e/action_test.go` scenarios.
export const FAKE_TEST_URL = "e2e-test-backend-mock-url:50051";

type StorageClient = Client<typeof StorageBackendDefinition>;

interface BackendContainer {
Expand Down Expand Up @@ -131,10 +125,6 @@ export default class DelegatedStorageService {
backendId: input.backend.id,
});
const backend = await this.getBackendContainer(input.backend.id);
if (!backend?.client) {
// TODO: remove after using a real backend in e2e tests.
continue;
}

const validateErr = this.validateInput(input, backend.validateSpec);
if (validateErr) {
Expand Down Expand Up @@ -179,11 +169,6 @@ export default class DelegatedStorageService {
backendId: input.backend.id,
});
const backend = await this.getBackendContainer(input.backend.id);
if (!backend?.client) {
// TODO(https://github.com/capactio/capact/issues/634): remove after using a real backend in e2e tests.
continue;
}

const validateErr = this.validateInput(input, backend.validateSpec);
if (validateErr) {
throw Error(
Expand Down Expand Up @@ -220,16 +205,6 @@ export default class DelegatedStorageService {
backendId: input.backend.id,
});
const backend = await this.getBackendContainer(input.backend.id);
if (!backend?.client) {
// TODO(https://github.com/capactio/capact/issues/634): remove after using a real backend in e2e tests.
result = {
...result,
[input.typeInstance.id]: {
key: input.backend.id,
},
};
continue;
}

const validateErr = this.validateInput(input, backend.validateSpec);
if (validateErr) {
Expand Down Expand Up @@ -274,10 +249,6 @@ export default class DelegatedStorageService {
backendId: input.backend.id,
});
const backend = await this.getBackendContainer(input.backend.id);
if (!backend?.client) {
// TODO(https://github.com/capactio/capact/issues/634): remove after using a real backend in e2e tests.
continue;
}

const validateErr = this.validateInput(input, backend.validateSpec);
if (validateErr) {
Expand Down Expand Up @@ -307,10 +278,6 @@ export default class DelegatedStorageService {
backendId: input.backend.id,
});
const backend = await this.getBackendContainer(input.backend.id);
if (!backend?.client) {
// TODO(https://github.com/capactio/capact/issues/634): remove after using a real backend in e2e tests.
continue;
}

const validateErr = this.validateInput(input, backend.validateSpec);
if (validateErr) {
Expand Down Expand Up @@ -341,10 +308,6 @@ export default class DelegatedStorageService {
backendId: input.backend.id,
});
const backend = await this.getBackendContainer(input.backend.id);
if (!backend?.client) {
// TODO(https://github.com/capactio/capact/issues/634): remove after using a real backend in e2e tests.
continue;
}

const validateErr = this.validateInput(input, backend.validateSpec);
if (validateErr) {
Expand Down Expand Up @@ -408,19 +371,9 @@ export default class DelegatedStorageService {
}
}

private async getBackendContainer(
id: string
): Promise<BackendContainer | undefined> {
private async getBackendContainer(id: string): Promise<BackendContainer> {
if (!this.registeredClients.has(id)) {
const spec = await this.storageInstanceDetailsFetcher(id);
if (spec.url === FAKE_TEST_URL) {
logger.debug(
"Skipping a real call as backend was classified as a fake one"
);
// TODO(https://github.com/capactio/capact/issues/634): remove after using a real backend in e2e tests.
return undefined;
}

logger.debug("Initialize gRPC BackendContainer", {
backend: id,
url: spec.url,
Expand Down Expand Up @@ -451,7 +404,7 @@ export default class DelegatedStorageService {
this.registeredClients.set(id, { client, validateSpec: storageSpec });
}

return this.registeredClients.get(id);
return this.registeredClients.get(id) as BackendContainer;
}

private static convertToJSONIfObject(val: unknown): string | undefined {
Expand Down
Loading