-
Notifications
You must be signed in to change notification settings - Fork 146
feat: handle default and supported values for building in source per workflow #418
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
feat: handle default and supported values for building in source per workflow #418
Conversation
# Don't build in source by default (backwards compatibility) | ||
if build_in_source is None: | ||
build_in_source = False | ||
|
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 recently added this in a previous PR to handle build-in-source defaults for custom makefile, but it's not needed anymore with the new class constant logic (BUILD_IN_SOURCE_BY_DEFAULT
)
aws_lambda_builders/workflow.py
Outdated
else: | ||
error_reason = "Unsupported value for build_in_source" | ||
|
||
raise WorkflowFailedError(workflow_name=self.NAME, action_name=None, reason=error_reason) |
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.
hmmm actually we might not want to fail. I might speak with UX about this but I think I'll change this to show a warning/log and use the default value for the workflow.
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.
For context: one of the reasons we might not want this to fail is that the --in-source
flag is applied to all functions and layers of a project for now. If there's one function/layer that doesn't support building in source, this would fail the whole build. If it just raised a warning and used the default value, users would still be able to build (although there might be less visibility into the warning message vs. an error message)
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.
Not blocking, but will leave my comments in there on same code path being executed on if and elif. There could be ways in which we make that cleaner.
Issue #, if available:
Description of changes:
Building in source
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.