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

fix: removing job due to required scopes not matching with token scope #408

Closed
wants to merge 1 commit into from

Conversation

akturis
Copy link

@akturis akturis commented Jul 2, 2024

fix: removing job due to required scopes not matching with token scope

fix: removing job  due to required scopes not matching with token scope
@recursivetree
Copy link
Contributor

I see that your PR lacks an explanation why these changes are required. Providing one would help understanding your motivation and make the whole process of merging smoother. If you haven't provided a description because your PR isn't ready for review yet, let us know and we'll take a look once it is ready.

Otherwise, I have a few questions:

  1. What is the reason that you want this change? Did you run into a problem or a bug?
  2. And what is the reasoning behind making it delete the job opposed to failing it?
  3. Wouldn't it make more sense to not schedule a job we can't process? Having a middleware as a last safeguard is fine imo, but if it is a last safeguard, it should fail like an error instead of deleting.

Also, the styleci bot has found a few isues with your code formattting. Could you please apply the changes it suggests?

'job_scopes' => $job->getScope(),
'character_id' => $job->getToken()->character_id,
]);
$job->delete();
Copy link
Contributor

Choose a reason for hiding this comment

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

So you want to delete the job rather than have it fail because the job ran without the required scopes?

These types of errors are actually very useful to server owners like myself, to know when things are going wrong. Deleting it behind a Warning-type log is not ideal IMO

@Crypta-Eve
Copy link
Member

I agree that we should not be deleting these jobs. This PR is related to eveseat/seat#910 however I think the more appropriate diagnosis is to prevent jobs from being queued if they are not scoped for the job

@Crypta-Eve Crypta-Eve closed this Jul 12, 2024
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.

4 participants