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

Add ability to configure timeout for Scans #273

Merged
merged 4 commits into from
Jun 9, 2023
Merged

Conversation

ghost
Copy link

@ghost ghost commented Apr 25, 2023

Description

Through the scan configuration a maximum number of seconds (timeoutSeconds) can be configured.

When a scan is triggered from a configuration this number of seconds will be added to the start time and saved as timeoutTime on the Scan object.

If while the Scan is running the current time exceeds the timeoutTime then the scan is automatically failed and any remaining AssetScans will be terminated.

If no timeoutTime is provided on a scan it will be automatically timed out 48 hours after the start time of the scan.

Type of Change

[ ] Bug Fix
[X] New Feature
[ ] Breaking Change
[ ] Refactor
[ ] Documentation
[ ] Other (please describe)

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests
  • Functionality is documented
  • All code style checks pass
  • New code contribution is covered by automated tests
  • All new and existing tests pass

@chrisgacsal chrisgacsal force-pushed the scan_timeout branch 2 times, most recently from 26f8e7e to 94219fb Compare June 7, 2023 15:26
@chrisgacsal chrisgacsal self-assigned this Jun 7, 2023
@chrisgacsal chrisgacsal marked this pull request as ready for review June 7, 2023 15:26
@chrisgacsal chrisgacsal requested a review from a team as a code owner June 7, 2023 15:26
@chrisgacsal chrisgacsal added enhancement New feature or request go Pull requests that update Go code labels Jun 7, 2023
Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I think we should drop the TimeoutTime logic now, its not great from an API UX perspective and StartTime + TimeoutSeconds satisfies the same flow in the code.

api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
@chrisgacsal
Copy link
Contributor

I think we should drop the TimeoutTime logic now, its not great from an API UX perspective and StartTime + TimeoutSeconds satisfies the same flow in the code.

Agreed. It felt weird to me as well. I thought that having the TimeoutTime could be helpful to have this field for API consumers so they do not need to calculate the actual timeout on their own. But doing the math on the client side should not be an issue.

Tehsmash and others added 2 commits June 8, 2023 17:00
Through the scan configuration a maximum number of seconds
(timeoutSeconds) can be configured.

When a scan is triggered from a configuration this number of seconds
will be added to the start time and saved as timeoutTime on the Scan
object.

If while the Scan is running the current time exceeds the timeoutTime
then the scan is automatically failed and any remaining AssetScans will
be terminated.

If no timeoutTime is provided on a scan it will be automatically timed
out 48 hours after the start time of the scan.
Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @chrisgacsal

@chrisgacsal chrisgacsal added this pull request to the merge queue Jun 9, 2023
Merged via the queue into main with commit 7dec60f Jun 9, 2023
5 checks passed
@chrisgacsal chrisgacsal deleted the scan_timeout branch June 9, 2023 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants