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

Add new demo env that uses faked execution server #187

Merged
merged 9 commits into from
Feb 17, 2023

Conversation

rtorrero
Copy link
Contributor

Initial implementation of a faker to generate data for the checks execution.

It achieves this by providing a implementation for the execution server that instead of triggering executions on agents uses the factories from the tests to generate such results.

@coveralls
Copy link
Collaborator

Coverage Status

Coverage: 96.309% (+1.07%) from 95.238% when pulling 55b720a on rtorrero:add-wanda-faker into 4c7985b on trento-project:main.

@coveralls
Copy link
Collaborator

coveralls commented Feb 15, 2023

Coverage Status

Coverage: 96.309% (+1.07%) from 95.238% when pulling be28e2f on rtorrero:add-wanda-faker into 4c7985b on trento-project:main.

@rtorrero rtorrero marked this pull request as ready for review February 15, 2023 16:36
def start_execution(execution_id, group_id, targets, env, config \\ []) do
create_fake_execution(execution_id, group_id, targets)
Process.sleep(2_000)
complete_fake_execution(execution_id, group_id, targets, :passing)
Copy link
Member

Choose a reason for hiding this comment

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

Can we randomize the result with https://hexdocs.pm/elixir/1.12/Enum.html#random/1 

You can skip the function parameter and do it directly there

Copy link
Contributor

@arbulu89 arbulu89 Feb 16, 2023

Choose a reason for hiding this comment

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

@fabriziosestito @rtorrero In our offline conversation, i just was saying, that we can have this execution result coming from the checks results, which now I see is fairly simple. It doesn't need to be random.
You just need to loop through the check results, check the result of them, and simply weight them (or some basic operation to find the worst check result).
Anyway, I'm fine on improving this on future iterations

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

I find the current implementation too dump honestly.
We need to think on having certain logic like:
If the expectations are true -> the result of the check is true, etc
We cannot have failed expectations and passing results.
Now that we will implement the new view of the results using all these data, it would look totally illogical.

I think that we need to invest much more on this.
If you want to do in a subsequent PR is still good.

config/demo.exs Outdated Show resolved Hide resolved
config/demo.exs Outdated Show resolved Hide resolved
demo/fake_server.ex Show resolved Hide resolved
demo/fake_server.ex Show resolved Hide resolved
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Thanks @rtorrero
I still think that there are unfinished things, but I'm fine merging this first to at least enable the demo environment, and we can work on it later at some point

config/demo.exs Outdated Show resolved Hide resolved
demo/fake_server.ex Show resolved Hide resolved
demo/fake_server.ex Show resolved Hide resolved
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

LGTM.
Some few typo,copy/paste and linting errors.
You need to add the demo folder to the .formatter.exs and .credo.exs file, to be included in the checks

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
demo/fake_server.ex Outdated Show resolved Hide resolved
demo/fake_server.ex Outdated Show resolved Hide resolved
demo/fake_server.ex Outdated Show resolved Hide resolved
@rtorrero rtorrero merged commit 701f0f2 into trento-project:main Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants