-
Notifications
You must be signed in to change notification settings - Fork 18
feat: add ACTOR_PERMISSION_LEVEL to the configuration
#689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as for JS SDK...
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.
danpoletaev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as for JS SDK, consider moving log message to worker 🙃
This reverts commit 2457458.
src/apify/_configuration.py
Outdated
| 'actor_permission_level', | ||
| 'apify_actor_permission_level', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really make sense to have multiple choices for a newly introduced variable? The aliases exist mainly for compatibility with outdated documentation 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I wasn't sure how the name was inferred, so it was just to be sure 😅.
So in this case, the actor_permission_level should be enough, right?
The name of the env var is ACTOR_PERMISSION_LEVEL and it is part of ActorEnvVars in apify-shared-python.
e.g.: I assume that it won't be prefixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we can keep just actor_permission_level and drop apify_actor_permission_level
vdusek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This adds the
ACTOR_PERMISSION_LEVELenvironmental variable asactor_permission_levelto the Configuration,Closes #690