Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Properly compare job/task state enums in JobHelper.check_current_job() #2004

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

DrChat
Copy link
Member

@DrChat DrChat commented Jun 1, 2022

Summary of the Pull Request

JobHelper.check_current_job() currently always reports a successful status because it compares state enums with string literals, which will never succeed (confusingly).
In addition, by default self.onefuzz.tasks.list will only list active tasks (excluding ones that are in a shutting down state).

PR Checklist

  • Applies to work item: #xxx
  • CLA signed. If not, go over here and sign the CLI.
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Info on Pull Request

This simply removes string literal comparisons in favor of lists returned by the state object itself.

Validation Steps Performed

Unsure - I don't know if there's any commands that can easily be ran to check this functionality.
This should be a simple enough code review.

@DrChat DrChat force-pushed the user/jusmoore/jobhelper_enums branch 2 times, most recently from 0a356f9 to 122364b Compare June 1, 2022 21:18
@Porges Porges force-pushed the user/jusmoore/jobhelper_enums branch from 122364b to 391782c Compare June 9, 2022 05:11
@Porges Porges force-pushed the user/jusmoore/jobhelper_enums branch from 391782c to f5bf51f Compare August 18, 2022 22:27
@Porges Porges force-pushed the user/jusmoore/jobhelper_enums branch from f5bf51f to 16adcd8 Compare August 22, 2022 03:17
@Porges Porges enabled auto-merge (squash) August 22, 2022 03:17
@Porges Porges disabled auto-merge August 22, 2022 03:17
@Porges
Copy link
Member

Porges commented Aug 22, 2022

Will merge after validation run.

@Porges Porges merged commit d83723b into microsoft:main Aug 22, 2022
@Porges
Copy link
Member

Porges commented Aug 22, 2022

Thanks for the contribution, @DrChat!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants