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

transfer of realm features to astarte_data_access #86

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

shinnokdisengir
Copy link

@shinnokdisengir shinnokdisengir commented Sep 10, 2024

In order to manage the database also from applications external to astarte, such as the various tests and products like astarte_dev_tool, the commands needed to manage the realms from the astarte_housekeeping to astarte_data_access

@shinnokdisengir shinnokdisengir force-pushed the feature/create-realm branch 7 times, most recently from 81ea6bb to 12136c1 Compare September 11, 2024 12:27
@shinnokdisengir shinnokdisengir marked this pull request as ready for review October 2, 2024 13:44
@shinnokdisengir shinnokdisengir force-pushed the feature/create-realm branch 6 times, most recently from 18f52aa to fae6e91 Compare October 3, 2024 10:14
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
lib/astarte_data_access/c_system.ex Outdated Show resolved Hide resolved
lib/astarte_data_access/entities/interface.ex Outdated Show resolved Hide resolved
lib/astarte_data_access/entities/keyspace.ex Outdated Show resolved Hide resolved
lib/astarte_data_access/xandra_utils.ex Outdated Show resolved Hide resolved
lib/astarte_data_access/xandra_utils.ex Outdated Show resolved Hide resolved
lib/astarte_data_access/xandra_utils.ex Show resolved Hide resolved
lib/astarte_data_access/xandra_utils.ex Outdated Show resolved Hide resolved
lib/astarte_data_access/xandra_utils.ex Outdated Show resolved Hide resolved
@shinnokdisengir shinnokdisengir force-pushed the feature/create-realm branch 4 times, most recently from e1d6df2 to 32c6413 Compare October 8, 2024 16:02
Moved old entities files into a specific `entities` folder, to
prepare realm.ex file creating

Moved files:
- device.ex
- interface.ex
- mappings.ex

Signed-off-by: Gabriele Ghio <[email protected]>
@shinnokdisengir shinnokdisengir force-pushed the feature/create-realm branch 2 times, most recently from 9fd9c83 to 3feae81 Compare October 8, 2024 16:32
CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 35 to 65
case XandraUtils.run(realm_name, fn conn, keyspace_name ->
astarte_keyspace_name = XandraUtils.build_keyspace_name!("astarte")

with :ok <- check_replication(conn, replication),
{:ok, replication_map_str} <- build_replication_map_str(replication) do
do_create_realm(
conn,
keyspace_name,
astarte_keyspace_name,
realm_name,
replication_map_str,
max_retention,
public_key_pem,
device_registration_limit,
realm_schema_version
)
end
end) do
:ok ->
:ok

{:error, reason} ->
Logger.warning("Cannot create realm: #{inspect(reason)}.",
tag: "realm_creation_failed",
realm: realm_name
)

{:error, reason}
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

The function call with many parameters inside a with inside a fn inside a case makes somewhat difficult to follow where the :ok/{:error, reason} below do come from.
I would suggest two changes:

  • Since we're inside a library, it is reasonable to think that the library user has more context regarding realm creation, so the caller of create_realm can decide whether to log or not. Removing the warning would also remove the need for case. This would be somewhat inconsistent with the rest of the library (we have lots of logs!) but more in line with the standard practice.
  • Generating astarte_keyspace_name and replication_map_str can be moved inside the with block in do_create_realm, as they're needed in every call.
    Essentially, this would turn into
Suggested change
case XandraUtils.run(realm_name, fn conn, keyspace_name ->
astarte_keyspace_name = XandraUtils.build_keyspace_name!("astarte")
with :ok <- check_replication(conn, replication),
{:ok, replication_map_str} <- build_replication_map_str(replication) do
do_create_realm(
conn,
keyspace_name,
astarte_keyspace_name,
realm_name,
replication_map_str,
max_retention,
public_key_pem,
device_registration_limit,
realm_schema_version
)
end
end) do
:ok ->
:ok
{:error, reason} ->
Logger.warning("Cannot create realm: #{inspect(reason)}.",
tag: "realm_creation_failed",
realm: realm_name
)
{:error, reason}
end
end
XandraUtils.run(
realm_name,
&do_create_realm(
&1,
realm_name,
replication,
max_retention,
public_key_pem,
device_registration_limit,
realm_schema_version
)
)
end

Comment on lines +67 to +82
case XandraUtils.run(realm_name, fn conn, keyspace_name ->
astarte_keyspace_name = XandraUtils.build_keyspace_name!("astarte")
do_delete_realm(conn, keyspace_name, astarte_keyspace_name, realm_name)
end) do
:ok ->
:ok

{:error, reason} ->
Logger.warning("Cannot delete realm: #{inspect(reason)}.",
tag: "realm_deletion_failed",
realm: realm_name
)

{:error, reason}
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

The above suggestion on error logging applies also here:

Suggested change
case XandraUtils.run(realm_name, fn conn, keyspace_name ->
astarte_keyspace_name = XandraUtils.build_keyspace_name!("astarte")
do_delete_realm(conn, keyspace_name, astarte_keyspace_name, realm_name)
end) do
:ok ->
:ok
{:error, reason} ->
Logger.warning("Cannot delete realm: #{inspect(reason)}.",
tag: "realm_deletion_failed",
realm: realm_name
)
{:error, reason}
end
XandraUtils.run(realm_name, fn conn, keyspace_name ->
astarte_keyspace_name = XandraUtils.build_keyspace_name!("astarte")
do_delete_realm(conn, keyspace_name, astarte_keyspace_name, realm_name)
end)

Note that it is not mandatory, but we can decide for one option or the other in the whole PR (I suggest to remove logs).

lib/astarte_data_access/xandra_utils.ex Show resolved Hide resolved
lib/astarte_data_access/entities/device.ex Outdated Show resolved Hide resolved
Created the realm.ex file, containing everything necessary for CRUD manipulation of `realms``:
- create
- listing
- deletion
- reading single realm

added functionality/small refactoring of `xandra_utils`.

Signed-off-by: Gabriele Ghio <[email protected]>
Added `Realm` tests:
- UT
- Create/Delete

Signed-off-by: Gabriele Ghio <[email protected]>
Edited CHANGELOG.md

- Realm database features.

Signed-off-by: Gabriele Ghio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants