-
Notifications
You must be signed in to change notification settings - Fork 61
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
[Core] Add opt-in integration resource provision service #1297
base: main
Are you sure you want to change the base?
Conversation
a5c6b85
to
0886913
Compare
802d38d
to
b00e679
Compare
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.
Nice work
left a few comments, overall seems like you've added some core functionality to the integration initialization which I think is really worth adding testing for to make sure everything is behaving as expected
async def _poll_integration_until_default_provisioning_is_complete( | ||
self, | ||
interval=15, |
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.
15 what?
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.
lets not use magic numbers
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.
can we do some kind of backoff or eventually mechanism? 15 seconds is huge time to wait for integration to finish set up
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.
good idea, will add
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.
Fixed!
@@ -68,6 +68,7 @@ class IntegrationConfiguration(BaseOceanSettings, extra=Extra.allow): | |||
initialize_port_resources: bool = True | |||
scheduled_resync_interval: int | None = None | |||
client_timeout: int = 60 | |||
use_provisioned_defaults: bool = 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.
are we sure we want to use only two options False
and True
? maybe worth changing the variable to provisioning_mode
- port
- local
which will allow us more flexability in the future?
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.
WDYM? Why would that be different?
e3b9410
to
78cf234
Compare
f"Fetching created integration and validating config, attempt {attempts+1}/{INTEGRATION_POLLING_RETRY_LIMIT}" | ||
) | ||
response = await self._get_current_integration() | ||
response_json = response.json() |
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.
response_json = response.json() | |
integration_response = response.json() |
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.
Fixed
if config != {}: | ||
return response_json | ||
|
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.
if config != {}: | |
return response_json | |
if config: | |
return response_json | |
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.
Fixed
if use_provisioned_defaults: | ||
json["provisionEnabled"] = use_provisioned_defaults | ||
|
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.
Fixed
super().__init__( | ||
f"Failed to retrieve integration config after {retries} attempts" |
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 mean that the integration won't continue to run after this issue happens
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.
Yes, and it'll be in the logs
f3d696d
to
1a40313
Compare
79e3d2a
to
152f355
Compare
11f99ed
to
bf41328
Compare
Description
What - Add a flow that verifies the
USE_PROVISIONED_DEFAULTS
and uses the provisioning resources accordinglyLogic:
Integration marked
use_provisioned_defaults
as true but org feature toggleUSE_PROVISIONED_DEFAULTS
isn't availableOrg feature toggle
USE_PROVISIONED_DEFAULTS
enabled but integration wasn't marked withuse_provisioned_defaults
Integration marked
use_provisioned_defaults
as true and org feature toggleUSE_PROVISIONED_DEFAULTS
is setType of change
All tests should be run against the port production environment(using a testing org).
Core testing checklist