feat: add ACTOR_PERMISSION_LEVEL to env var interface
#516
Conversation
stepskop
added
the
t-core-services
packages/apify/src/actor.ts
Outdated
| log.info('Permissions', { | ||
| actorPermissionLevel: this.getEnv().actorPermissionLevel, | ||
| }); |
There was a problem hiding this comment.
I am not sure if this should not be somehow conditional, as most of the time it would be defined only when run on Apify. Locally, it won't be defined if the user doesn't do so.
However, I can imagine some cases when my code depends on the permission level, so I am passing it "artificially" even locally to test things out.
There was a problem hiding this comment.
I was actually thinking that we'd print it via worker. That would be more universal 🤔
There was a problem hiding this comment.
I thought that the original intention was to put it in the system message (= SDK), but when implementing it, I decided not to put it there, as the system message consists of more "low level" information.
I guess we can log it through Worker.
There was a problem hiding this comment.
I thought system message means worker 😅
There was a problem hiding this comment.
+1 for logging this elsewhere, locally this IMO doesn't add much value. It's either unset, i.e., this line will print Permissions { actorPermissionLevel: null }, or I pass it manually = I did that intentionally, I know what permission level I'm on.
There was a problem hiding this comment.
Would also vote for worker or in worst case maybe add it to log.info('System info', getSystemInfo());
There was a problem hiding this comment.
I wouldn't add it to the system message as I said here: #516 (comment)
Lets move to to Worker 👍
stepskop
changed the title
actorPermissionLevel to env var interfaceACTOR_PERMISSION_LEVEL to env var interface
packages/apify/src/actor.ts
Outdated
| log.info('Permissions', { | ||
| actorPermissionLevel: this.getEnv().actorPermissionLevel, | ||
| }); |
There was a problem hiding this comment.
I was actually thinking that we'd print it via worker. That would be more universal 🤔
barjin
left a comment
barjin
left a comment
There was a problem hiding this comment.
I don't have any blockers other than the logging, so please reconsider that and feel free to merge. Thank you!
Note the parity tracking issue in the Python SDK.
packages/apify/src/actor.ts
Outdated
| log.info('Permissions', { | ||
| actorPermissionLevel: this.getEnv().actorPermissionLevel, | ||
| }); |
There was a problem hiding this comment.
+1 for logging this elsewhere, locally this IMO doesn't add much value. It's either unset, i.e., this line will print Permissions { actorPermissionLevel: null }, or I pass it manually = I did that intentionally, I know what permission level I'm on.
packages/apify/src/actor.ts
Outdated
| /** | ||
| * [Permission level](https://docs.apify.com/platform/actors/development/permissions) the Actor is run under. (ACTOR_PERMISSION_LEVEL) | ||
| */ | ||
| actorPermissionLevel: string | null; |
There was a problem hiding this comment.
Since this is an enum, I'm wondering if we could type this strictly ('LIMITED_PERMISSIONS' | 'FULL_PERMISSIONS' | null).
It would help users with adoption, but might require some well-placed casts.
There was a problem hiding this comment.
I typed it using the ACTOR_PERMISSION_LEVEL from @apify/consts
danpoletaev
left a comment
danpoletaev
left a comment
There was a problem hiding this comment.
Let's decide whether we'll move it to worker
packages/apify/src/actor.ts
Outdated
| log.info('Permissions', { | ||
| actorPermissionLevel: this.getEnv().actorPermissionLevel, | ||
| }); |
There was a problem hiding this comment.
Would also vote for worker or in worst case maybe add it to log.info('System info', getSystemInfo());
This adds the
ACTOR_PERMISSION_LEVELenvironmental variable asactorPermissionLevelto theApifyEnvinterface.