Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Draft: Add BinaryApp application type and p47 example deployment #23
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
Draft: Add BinaryApp application type and p47 example deployment #23
Changes from all commits
ce90909
30cc43d
69fd31b
939af01
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 35 in src/deploy_tools/app_builder.py
src/deploy_tools/app_builder.py#L34-L35
Check warning on line 53 in src/deploy_tools/app_builder.py
src/deploy_tools/app_builder.py#L53
Check warning on line 115 in src/deploy_tools/app_builder.py
src/deploy_tools/app_builder.py#L115
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.
We should use the same folder for all entrypoints. Maybe even rename it to bin?
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.
Cool - lets do that.
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.
This is still todo - right now I'm using
entrypoints
only.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.
I would recommend calling it 'bin_folder' if you do decide to rename things, as 'binary_folder' isn't commonly used. I'm happy to make this change myself, later, if you want.
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.
Is there anything checking that app names are unique?
Check warning on line 120 in src/deploy_tools/app_builder.py
src/deploy_tools/app_builder.py#L118-L120
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.
Buffer size should be module constant
Check warning on line 130 in src/deploy_tools/app_builder.py
src/deploy_tools/app_builder.py#L122-L130
Check warning on line 132 in src/deploy_tools/app_builder.py
src/deploy_tools/app_builder.py#L132
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.
Permissions should at least be a module constant. At the moment, it is all tidied away in templater.py, but this obviously breaks that and it may be sensible to move these permissions to another location.
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.
I'm not entirely clear how you want to handle this. So I've left it as is for now in #26 - see #26 (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.
I've added an extra comment there - I recommend at least a module-level constant.
Check warning on line 26 in src/deploy_tools/layout.py
src/deploy_tools/layout.py#L26
Check warning on line 50 in src/deploy_tools/layout.py
src/deploy_tools/layout.py#L50
Check warning on line 85 in src/deploy_tools/layout.py
src/deploy_tools/layout.py#L85