Configurator versioning #908
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Nico-Sanchez
force-pushed
the
add-configurator-hash-to-arena-version
branch
from
f65a08e to
38ae9a4
Compare
Nico-Sanchez
force-pushed
the
add-configurator-hash-to-arena-version
branch
from
38ae9a4 to
f02d8f6
Compare
AminArria
reviewed
Sep 10, 2024
| iex> get_configuration_hash_version() | ||
| "A6F0CC6917D195AB8A03129ACBE1FA48364845B8" | ||
| """ | ||
| def get_configuration_hash_version do |
There was a problem hiding this comment.
Why not :erlang.term_to_binary/1 the map configuration (or whatever subfields we actually want)?
There was a problem hiding this comment.
Because the query order of everything changes the hash everytime, and we only want to change the hash when the desired values change.
We can discuss a better approach in Slack.
AminArria
reviewed
Sep 10, 2024
| |> Enum.map(fn config -> sum_shape_coordinates(config) end) | ||
| |> Enum.reduce(fn coordinates_last, coordinates_current -> Decimal.add(coordinates_last, coordinates_current) end) | ||
| |> Decimal.to_string() | ||
| |> (fn coordinates -> :crypto.hash(:sha, coordinates) |> Base.encode16() end).() |
There was a problem hiding this comment.
Just selected one at random. Any suggestions?
There was a problem hiding this comment.
Nope, was just curious. Also, don't think is worth discussing for this use case
tvillegas98
previously approved these changes
Sep 12, 2024
manucamejo
previously approved these changes
Sep 12, 2024
manucamejo
added
the
waiting on PR
manucamejo
dismissed stale reviews from tvillegas98 and themself
via
651b5e8
manucamejo
reviewed
Sep 13, 2024
| @@ -8,8 +8,11 @@ defmodule ArenaWeb.HealthController do | |||
| end | |||
|
|
|||
| def version(conn, _params) do | |||
| arena_version = Application.spec(:arena, :vsn) |> to_string() | |||
| configurator_version = GameBackend.Configuration.get_configuration_hash_version() | |||
There was a problem hiding this comment.
Be careful, this controller is duplicated in both arena and gateway apps.
We should distinct them somehow or see if both are needed.
manucamejo
requested changes
Sep 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Warning
This won't be used until client's PR gets merged -> https://github.com/lambdaclass/champions_of_mirra/pull/2134
Motivation
Closes #918
We need a way to differentiate between configs.
If we change a map configuration (e.g. delete an obstacle or edit its size/position) the version should change to alert the clients.
Summary of changes
How to test it?
Make your client (https://github.com/lambdaclass/champions_of_mirra/pull/2134) point to localhost or central testing.
Then, update the version to match the configuration's.
Play a bit with map configuration params, it should ONLY change the version when something we take into account is changed (like an obstacle position, or a circle radius).
Checklist