feat: respect input schema defaults in Actor.getInput()#409
Conversation
github-actions
Bot
added
t-tooling
barjin
left a comment
barjin
left a comment
There was a problem hiding this comment.
Few nits, but looks alright to me otherwise, thank you!
|
|
||
| export const readInputSchema = (): Dictionary | null => { | ||
| const localConfig = readJSONIfExists( | ||
| join(process.cwd(), ACTOR_SPECIFICATION_FOLDER, LOCAL_CONFIG_NAME), |
There was a problem hiding this comment.
process.cwd() will likely break if the Dockerfile entry point is not npm (--prefix=app/) start, but e.g. node app/main.js
But I'm not sure how else we would refer to these files 🤔 I guess it's okay to live with this, especially since on Platform, we load this via API.
There was a problem hiding this comment.
I only gave it a quick look, but it looks like we don't copy the .actor directory in our Dockerfiles anyway, so for the "build a Docker image and run it outside of the platform" use case, the answer to "But I'm not sure how else we would refer to these files" is probably "tough luck lol" 🤷
But if we add a nice error log when there's not input schema to be found, yeah, we can live with that.
There was a problem hiding this comment.
I mean outside process.cwd, we have no way of finding these files... Maybe if we just walk up from process.cwd but I would rather... not :D
|
|
||
| export const readInputSchema = (): Dictionary | null => { | ||
| const localConfig = readJSONIfExists( | ||
| join(process.cwd(), ACTOR_SPECIFICATION_FOLDER, LOCAL_CONFIG_NAME), |
There was a problem hiding this comment.
I only gave it a quick look, but it looks like we don't copy the .actor directory in our Dockerfiles anyway, so for the "build a Docker image and run it outside of the platform" use case, the answer to "But I'm not sure how else we would refer to these files" is probably "tough luck lol" 🤷
But if we add a nice error log when there's not input schema to be found, yeah, we can live with that.
B4nan
changed the title
Actor.getInput()
|
@vladfrangu there are some unresolved comments from others too, I'd like to ship next SDK soon, including this PR, let's try to resolve them asap |
|
If this is all good from you guys, lets merge it and release it! |
| return null; | ||
| }; | ||
|
|
||
| export const kNoActorInputSchemaDefined = Symbol.for( |
There was a problem hiding this comment.
this is really weird naming. also, do we really need to export this symbol? if so, it should be marked as @ignore so it wont end up in the docs
There was a problem hiding this comment.
I just used naming similar to what I've seen in node (and we need it exported to check for it in actor.ts). I'll slap on the ignore tag in a sec
There was a problem hiding this comment.
i think we already discussed this some time ago. its weird, confusing, and unnecessary, lets not adopt it just because people maintaining node use it.
btw it looks like this file is not exported from the barrel so its fine i guess
5 participants
Way too overdue, closes #287