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

Remove Indivuidual Response #1570

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Remove Indivuidual Response #1570

wants to merge 26 commits into from

Conversation

Yuyuutsu
Copy link
Contributor

@Yuyuutsu Yuyuutsu commented Jan 7, 2025

What is the context of this PR?

Remove Individual Response (IR) Functionality from Runner
New Test was added for LogPublisher and some tests were removed too.

How to review

Check if Runner works as expected.
Tests Pass and changes make sense.
Check if all references to IR have been removed.

Checklist

  • New static content marked up for translation
  • Newly defined schema content included in eq-translations repo

@Yuyuutsu Yuyuutsu marked this pull request as ready for review January 13, 2025 08:45
@Yuyuutsu Yuyuutsu requested a review from a team as a code owner January 13, 2025 08:45
@@ -50,3 +51,17 @@ def test_resolving_message_raises_exception_on_error(publisher):
b"test-message",
fulfilment_request_transaction_id=str(uuid4()),
)


def test_log_publisher_publish():
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we needed this new test?

Copy link
Contributor Author

@Yuyuutsu Yuyuutsu Jan 16, 2025

Choose a reason for hiding this comment

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

LogPublisher was covered intest_individual_response.py but was not tested directly.

@@ -15,7 +15,6 @@ EQ_REDIS_HOST=localhost
EQ_REDIS_PORT=6379
EQ_SECRETS_FILE=dev-secrets.yml
EQ_KEYS_FILE=dev-keys.yml
EQ_INDIVIDUAL_RESPONSE_POSTAL_DEADLINE=2021-04-28T14:00:00+00:00
Copy link
Contributor

@berroar berroar Jan 15, 2025

Choose a reason for hiding this comment

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

Removed the env vars here but they are passed in our workflows to the deploy_app.yaml file. So will also need to update the pipelines and terraform and test deploying runner with the vars removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and Tested. New PRs have been added to the card:

@berroar berroar added the dont merge Don't merge this PR label Jan 27, 2025
Copy link
Contributor

@petechd petechd left a comment

Choose a reason for hiding this comment

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

To fully test this we need to change the validator branch as well (the tag=latest line) and revert after the Validator changes are merged?

@@ -30,7 +30,8 @@ <h1>{{ content.summary.title }}</h1>
{% set add_link_text = summary.add_link_text %}
{% set empty_list_text = summary.empty_list_text %}
{% set list_title = summary.title %}
<div {{ 'class="ons-u-mb-l"' if not loop.last }} data-qa="{{ summary.list_name }}-list-summary">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this change added? Seems unrelated to your changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I think was an accidental reformat. Reverted now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont merge Don't merge this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants