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 detailed experiment status #663

Merged
merged 11 commits into from
Jan 30, 2025
Merged

Conversation

javiermtorres
Copy link
Contributor

What's changing

Adds detailed experiment status depending on the component job status.

Closes #571

How to test it

Integration tests are supplied with the change. Manually, the status can be retrieved and checked while the component jobs are cycling through their RUNNING and SUCCEEDED/FAILED status.

Additional notes for reviewers

N/A

I already...

  • Tested the changes in a working environment to ensure they work as expected
  • Added some tests for any new functionality
  • Updated the documentation (both comments in code and product documentation under /docs)
  • Checked if a (backend) DB migration step was required and included it if required

@github-actions github-actions bot added backend api Changes which impact API/presentation layer schemas Changes to schemas (which may be public facing) labels Jan 17, 2025
@javiermtorres
Copy link
Contributor Author

Note: this PR is based on #604 . The real amount of files changed is much smaller. Please don't review until this other PR is merged.

@javiermtorres javiermtorres changed the title Issue 571 add experiment status Add detailed experiment status Jan 17, 2025
@javiermtorres javiermtorres force-pushed the issue-571-add-experiment-status branch from 43fc199 to fd4da0b Compare January 21, 2025 08:26
@javiermtorres javiermtorres marked this pull request as ready for review January 21, 2025 08:26
Copy link
Member

@aittalam aittalam left a comment

Choose a reason for hiding this comment

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

TY @javiermtorres ! I added minor fixes to be applied before merging.
Blocking this just because I want to make sure the above are already covered by tests before merging, but if you can confirm this I am happy to approve it!

@javiermtorres javiermtorres force-pushed the issue-571-add-experiment-status branch from 7f1f9ed to 80c57e4 Compare January 22, 2025 19:30
Copy link
Member

@aittalam aittalam left a comment

Choose a reason for hiding this comment

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

TY @javiermtorres for implementing the new logic!
This looks good to me, I am happy to preapprove it. Re: impact on the current UI, there will be none as this service is hit only by the new endpoint

@javiermtorres javiermtorres merged commit a7ad438 into main Jan 30, 2025
13 checks passed
@javiermtorres javiermtorres deleted the issue-571-add-experiment-status branch January 30, 2025 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Changes which impact API/presentation layer backend schemas Changes to schemas (which may be public facing)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add get experiment status + tests
3 participants