Refactor OS installation progress #591
Conversation
lib/services/common-api-presenter.js
Outdated
| error: error | ||
| logger.error('Unable to render error profile.', { | ||
| error: error, | ||
| profileName: name, |
yyscamper
force-pushed
the
refactor-os-progress
branch
from
1daab9d to
8828985
Compare
|
test this please |
|
👍 |
|
|
||
| # The progress notification is just something nice-to-have, so progress notification failure should | ||
| # never impact the normal installation process | ||
| <% if( typeof progressMilestones !== 'undefined' && progressMilestones.enterProfileUri ) { %> |
There was a problem hiding this comment.
this code line is mixed in comments, the comments below could be indented
lib/services/common-api-presenter.js
Outdated
| logger.error('Unable to render error template.', { | ||
| macaddress: options.macaddress, | ||
| error: error | ||
| logger.error('Unable to render error profile.', { |
There was a problem hiding this comment.
the logs should be "Unable to render boilerplate.ipxe" ?
There was a problem hiding this comment.
The error occurs when both the input profile & boilerplate.ipxe rendering fail, but the error input profile is really something we care while analyzing log, so I don't think your log message is better than the original one.
lib/api/2.0/notification.js
Outdated
| notificationPost: notificationPost, | ||
| notificationProgressGet: notificationProgressGet | ||
| notificationProgressPost: notificationProgressPost, | ||
| //NOTE: in some environment, the HTTP client doesn't support POST, so we have to define |
There was a problem hiding this comment.
'environments' could you help provide the example 'ipxe' explicitly for understanding
static/monorail-2.0-sb.yaml
Outdated
| # to compromise to define below GET api to update progress information, but actually it does the | ||
| # job of verb "POST" | ||
| get: | ||
| operationId: notificationProgressGet |
There was a problem hiding this comment.
how about using 'notificationProgressPost' directly, it could avoid confusion in two places( another is "notificationProgressGet: notificationProgressPost" ) and avoid you have to comments in two places.
lib/api/2.0/notification.js
Outdated
| notificationProgressGet: notificationProgressGet | ||
| notificationProgressPost: notificationProgressPost, | ||
| //NOTE: in some environment, the HTTP client doesn't support POST, so we have to define | ||
| //a ugly API which use verb GET to update the progress information |
| Task instance identifier | ||
| required: true | ||
| The identifier of task instance which the progress applies to | ||
| required: false |
There was a problem hiding this comment.
Without taskId, progress actually can't work, suppose it is required.
There was a problem hiding this comment.
Actually if the progress data is embedded in http request body, then the taskId is optional in query.
| in: query | ||
| description: | | ||
| The identifier of task instance which the progress applies to | ||
| required: false |
1. Directly reference the milestone definition rather than the raw value. 2. Change all progress notification to be sent via URI /notification/progress 3. Explain the strange implemention for GET /notification/progress 4. Remove totalSteps/currentStep, always use the typic variable maximum/value
yyscamper
force-pushed
the
refactor-os-progress
branch
from
8828985 to
48d6b10
Compare
|
jenkins: test this please |
6 participants
jenkins: depends on RackHD/on-tasks#407
@pengz1 @lanchongyizu