scan estimation apiserver #559
Conversation
| type: array | ||
| items: | ||
| type: string | ||
| nullable: true |
There was a problem hiding this comment.
Lets add message and reason here too, that way if its Failed due to aborted, timeout or error we can tell the difference
| state: | ||
| description: The lifecycle state of this scan estimation. | ||
| type: string | ||
| enum: | ||
| - Pending | ||
| - Discovered | ||
| - InProgress | ||
| - Aborted | ||
| - Failed | ||
| - Done | ||
| stateMessage: | ||
| description: Human-readable message indicating details about the last state transition. | ||
| type: string | ||
| stateReason: | ||
| description: Machine-readable, UpperCamelCase text indicating the reason for the condition's last transition. | ||
| type: string | ||
| enum: | ||
| - Aborted | ||
| - TimedOut | ||
| - OneOrMoreAssetFailedToEstimate | ||
| - DiscoveryFailed | ||
| - Unexpected | ||
| - NothingToEstimate | ||
| - Success |
There was a problem hiding this comment.
Lets collapse this into its own ScanEstimationState object like what we're doing with the AssetScanEstimationState. This is the approach we're planning to take across all objects soon once someone is able to pick up the refactor work: https://github.com/openclarity/vmclarity/pull/389/files
There was a problem hiding this comment.
Maybe we should think about a general State object that will be used across all resources instead of defining a State for each object
There was a problem hiding this comment.
It would be nice to have a general state object, however we still want to have enums for state reason and state that are different for each type. It is possible to do that in openapi schema using "allOf" but the generator doesn't handle it well.
| type: string | ||
| readOnly: true | ||
| scanEstimation: | ||
| $ref: '#/components/schemas/ScanEstimation' |
There was a problem hiding this comment.
I don't think we need this one for ScanEstimation.
There was a problem hiding this comment.
Why not? It is returned in case of conflict in POST/PATCH
There was a problem hiding this comment.
There is no conflict case for a ScanEstimation as far as I know
| description: Total cost of the scan ($) | ||
| type: number | ||
| format: float | ||
| recipe: |
There was a problem hiding this comment.
I don't think recipe is the correct terminology here, costBreakdown would be easier to understand
| summary: | ||
| $ref: '#/components/schemas/ScanEstimationSummary' | ||
| revision: | ||
| type: integer |
There was a problem hiding this comment.
We're missing the relationship to the AssetScanEstimations which let us $expand it something like:
assetScanEstimations:
description: AssetScanEstimations which make up this ScanEstimation
type: array
items:
$ref: '#/components/schemas/AssetScanEstimationRelationship'
There was a problem hiding this comment.
Why do we need this if we already have assetIds? we can get the assetScanEstimation from the asset id and the scanEstimation id, no?
There was a problem hiding this comment.
We want to be able to GET /scanEstimation/<uuid>?$expand=AssetScans to get a single response that has all the estimations for all the assets. Otherwise we have to do first query the scanEstimation and then make N requests to get all the assetScanEstimations.
| Fields: odatasql.Schema{ | ||
| "id": odatasql.FieldMeta{FieldType: odatasql.PrimitiveFieldType}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
No need for this to exist, AssetScanEstimation should just have:
"scanEstimation": odatasql.FieldMeta{
FieldType: odatasql.RelationshipFieldType,
RelationshipSchema: scanEstimationSchemaName,
RelationshipProperty: "id",
},
| assetScanTemplate: | ||
| $ref: '#/components/schemas/AssetScanTemplate' | ||
| revision: | ||
| type: integer |
There was a problem hiding this comment.
We should add StartTime/EndTime here so that we can use them for timeout purposes.
| return sendError(ctx, http.StatusNotFound, fmt.Sprintf("asset scan estimation was not found. assetScanEstimationID=%v: %v", assetScanEstimationID, err)) | ||
| } | ||
| return sendError(ctx, http.StatusInternalServerError, fmt.Sprintf("failed to get asset scan estimation. assetScanEstimationID=%v: %v", assetScanEstimationID, err)) | ||
| } |
There was a problem hiding this comment.
No need for this the DB layer will check this for you as part of the Update function
| return sendError(ctx, http.StatusNotFound, fmt.Sprintf("asset scan estimation was not found. assetScanEstimationID=%v: %v", assetScanEstimationID, err)) | ||
| } | ||
| return sendError(ctx, http.StatusInternalServerError, fmt.Sprintf("failed to get asset scan estimation. assetScanEstimationID=%v: %v", assetScanEstimationID, err)) | ||
| } |
There was a problem hiding this comment.
Ditto, just need to move the ErrRecordNotFound error handling to the block below here.
| var validationErr *common.BadRequestError | ||
| var conflictErr *common.BadRequestError | ||
| var preconditionFailedErr *databaseTypes.PreconditionFailedError | ||
| switch true { |
There was a problem hiding this comment.
no need for "true" here switch in go can be used like:
switch {
case condition1:
case condition2:
}
|
Moved to #603 |
Description
Resolve #586
Discussion: #465
Adding the ability to post a new scan estimation request which will perform a cost estimation of a scan/assetScan according to the scan configuration (scan scope) and previous asset scans statistics.
Adds 2 new resources: ScanEstimation and AssetScanEstimation with corresponding API endpoints.
Also added 2 new tables in the DB.
2 new reconcile controllers for these new resources are added in the orchestrator PR: #560
The cost calculation implementation for AWS will be added soon.
Type of Change
[ ] Bug Fix
[* ] New Feature
[ ] Breaking Change
[ ] Refactor
[ ] Documentation
[ ] Other (please describe)