[#13644] Remove GAE Dependence for Cron Jobs#13690
[#13644] Remove GAE Dependence for Cron Jobs#13690Nsohko wants to merge 25 commits intoTEAMMATES:masterfrom
Conversation
…o sai/migrate-cron-from-gae
…/teammates into sai/migrate-cron-from-gae
|
Hi @Nsohko, thank you for your interest in contributing to TEAMMATES!
Please address the above before we proceed to review your PR. |
|
Hi @DhiraPT, can I get your review on this before I ping Samuel |
DhiraPT
left a comment
DhiraPT
left a comment
There was a problem hiding this comment.
Based on @samuelfangjw 's comment in the previous PR, most of the issues have been addressed. However, you have not renamed InternalRequestAuth and addressed the wrong pattern in LocalTaskQueueService (not the right layer to validate secrets as a core responsibility)
|
Hi @DhiraPT, thank you for the review @samuelfangjw Please let me know what would be a more apt name for I will rename |
@Nsohko sorry I forgot about this. How about |
| public boolean canAccessAsAdminOrInternalService() { | ||
| return isAdmin || isInternalService; | ||
| } |
There was a problem hiding this comment.
Could we move this directly into InternalServiceAction? This is not really the responsibility of the UserInfo class.
3 participants
Fixes #13644
Remove GAE dependence for Cron Jobs.
Address Samuel's feedback on PR #13658