-
Notifications
You must be signed in to change notification settings - Fork 56
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
[Framework] Always initialize port default mappings in case where no other mapping is configured #700
[Framework] Always initialize port default mappings in case where no other mapping is configured #700
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.
Few comments
also you are missing changelog and version bump
please bump it by minor and not by patch like usual i want to start managing the versions normally
@@ -73,7 +73,8 @@ async def _initialize_required_integration_settings( | |||
port_app_config=default_mapping, | |||
) | |||
except httpx.HTTPStatusError as err: | |||
raise Exception(f"Failed to apply default mapping: {err.response.text}.") | |||
logger.error("Failed to apply default mapping") |
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.
logger.error("Failed to apply default mapping") | |
logger.error(f"Failed to apply default mapping: {err.response.text}.") |
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.
that was the previous state, don't think it's needed to both log the error text and raise the whole exception
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.
The whole exception wont log the response text
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.
more comments
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
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.
🌊 dont forget to change the changelog date
Description
What - Always initialize port default mappings in case where no other mapping is configured
Why - Integration that initializes with empty mapping wont do enything in resync and will cause confusion with our clients, furthermore integration that runs 'ONCE' will take no effect with initializeportresources=false
How - reorder the initialization proccess
Type of change
Please leave one option from the following and delete the rest:
Screenshots
Include screenshots from your environment showing how the resources of the integration will look.
API Documentation
Provide links to the API documentation used for this integration.