Add SageMaker-UI poststartup endpoint patch #108
Conversation
| return this._handleWebExtensionResource(req, res, parsedUrl); | ||
| } | ||
| + if (pathname === this._postStartupScriptRoute) { | ||
| + return this._handlePostStartupScriptInvocation(req, res); |
There was a problem hiding this comment.
Spacing seems to be off here.
| res.setHeader('Content-Type', 'application/json'); | ||
| res.end(JSON.stringify({ 'success': 'true' })); | ||
| } else { | ||
| serveError(req, res, 404, 'Poststartup script file not found at ' + postStartupScripPath); |
There was a problem hiding this comment.
Wld this block anything on the HostAgent?
There was a problem hiding this comment.
HostAgent will handle all non-OK (non-200) responses and will not cause the app to fail.
I am adding that change in LLHA to handle NonOKResponse.
|
|
||
| res.statusCode = 200; | ||
| res.setHeader('Content-Type', 'application/json'); | ||
| res.end(JSON.stringify({ 'success': 'true' })); |
There was a problem hiding this comment.
Are we expecting the success value as boolean or string? This is giving as string value true
There was a problem hiding this comment.
I can change this to boolean true, but we dont use it.
There was a problem hiding this comment.
Lets follow the norm that we use in JL IDE.
| } | ||
| if (pathname === this._postStartupScriptRoute) { | ||
| return this._handlePostStartupScriptInvocation(req, res); | ||
| } |
| } | ||
| + if (pathname === this._postStartupScriptRoute) { | ||
| + return this._handlePostStartupScriptInvocation(req, res); | ||
| + } |
| /** | ||
| * Handles API requests to run the post-startup script in SMD. | ||
| */ | ||
| private async _handlePostStartupScriptInvocation(req: http.IncomingMessage, res: http.ServerResponse): Promise<void> { |
There was a problem hiding this comment.
Hey Mrunal - just thinking - i dont think we should even run through this function for SM AI side. Using the Env variable - lets restrict this SMUS only.
| } | ||
| if (pathname === this._postStartupScriptRoute) { | ||
| // Only trigger post-startup script invocation for SageMakerUnifiedStudio app. | ||
| if (process.env['SERVICE_NAME'] != 'SageMakerUnifiedStudio') { |
There was a problem hiding this comment.
We shld handle this in _handlePostStartupScriptInvocation method.
Thanks for the thorough testing. |
| const logStream = fs.createWriteStream(logPath, { flags: 'a' }); | ||
|
|
||
| // Only trigger post-startup script invocation for SageMakerUnifiedStudio app. | ||
| if (process.env['SERVICE_NAME'] != 'SageMakerUnifiedStudio') { |
There was a problem hiding this comment.
make constant or enum SageMakerUnifiedStudio. Also I think this check should be lifted into the handle function.
There was a problem hiding this comment.
I would like to keep it in specific function to handle the use cases as needed than in handle function whose only thing is to route the request itself.
|
|
||
| // Only trigger post-startup script invocation for SageMakerUnifiedStudio app. | ||
| if (process.env['SERVICE_NAME'] != 'SageMakerUnifiedStudio') { | ||
| return serveError(req, res, 400, 'Bad request.'); |
There was a problem hiding this comment.
400 is for malformed requests, this should be 403 forbidden with message like "This API is only accessible to Maxdome clients"
There was a problem hiding this comment.
Make sense. We can keep it simple 403 with message as Forbidden
| res.setHeader('Content-Type', 'application/json'); | ||
| res.end(JSON.stringify({ 'success': 'true' })); | ||
| } else { | ||
| serveError(req, res, 404, 'Poststartup script file not found at ' + postStartupScripPath); |
There was a problem hiding this comment.
404 is for if the file is part of the API url path and client is explicitly requesting it. This should be 500 internal error.
| // Adding 0o755 to make script file executable | ||
| fs.chmodSync(postStartupScripPath, 0o755); | ||
|
|
||
| const subprocess = spawn('bash', [`${postStartupScripPath}`], { cwd: '/' }); |
There was a problem hiding this comment.
is there a public link where we can view this script? I don't have access to the link in the description.
There was a problem hiding this comment.
No, its in our private SMD repo
mndeshmu97
force-pushed
the
feature/smus
branch
from
563277e to
3cd6872
Compare
3 participants
Description
Testing Done
Issue #, if available:
Description of changes:
/api/poststartupendpoint for the SageMaker Unified Studio user case that runs the post startup script.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.