Skip to content

Remove inline RISC job#10826

Merged
vrajmohan merged 1 commit intomainfrom
remove-inline-risc-job
Jun 20, 2024
Merged

Remove inline RISC job#10826
vrajmohan merged 1 commit intomainfrom
remove-inline-risc-job

Conversation

@vrajmohan
Copy link
Contributor

No description provided.

@vrajmohan vrajmohan requested a review from zachmargolis June 18, 2024 14:56
@vrajmohan vrajmohan marked this pull request as draft June 18, 2024 15:37
end
end

def inline?
Copy link
Contributor

Choose a reason for hiding this comment

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

if we're following through with this we should also update the logic in http_push.rb and remove the risc_notifications_active_job_enabled feature flag as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I saw that when the tests failed on CI. Looking into it.

@vrajmohan vrajmohan marked this pull request as ready for review June 18, 2024 18:58
@vrajmohan vrajmohan force-pushed the remove-inline-risc-job branch 4 times, most recently from c95eeea to f787e33 Compare June 20, 2024 13:24
@vrajmohan vrajmohan requested a review from zachmargolis June 20, 2024 13:25
Copy link
Contributor

Choose a reason for hiding this comment

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

let's inline the job_arguments hash here

Copy link
Contributor

Choose a reason for hiding this comment

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

this was the last usage of this feature flag, so now let's fully remove it everywhere by taking it out of application.yml.default and identity_config.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was what I intended with my commit comment -> How -> 3rd bullet. Did you mean something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Feature flags are safe to remove in the codebase in the same PR that stop using them, so no need for a second PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I should have remembered that.

**Why**:
RISC notifications are delivered through an ActiveJob; inline
invocation is no longer used.

**How**:
1. Remove references to risc_notifications_active_job_enabled in code
   and assume it to be true
2. Refactor tests to assert that RiscDeliveryJob is scheduled with the
   correct arguments

changelog: Internal, RISC, Remove obsolete code
@vrajmohan vrajmohan force-pushed the remove-inline-risc-job branch from 4ae5546 to e68aea5 Compare June 20, 2024 18:20
@vrajmohan vrajmohan merged commit 1141bd4 into main Jun 20, 2024
@vrajmohan vrajmohan deleted the remove-inline-risc-job branch June 20, 2024 20:48
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.

2 participants