Skip to content
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

Add wait for status update #186

Merged

Conversation

deadlycoconuts
Copy link
Contributor

Context

This update to the Turing SDK adds commits to the existing release of Turing SDK (v0.2.0), which serve to introduce a version attribute in the Router SDK. This version attribute (as well as a router's other attributes) gets updated when wait_for_status gets called on a Router instance. When wait_for_status returns successfully, the version attribute would be expected to contain the current router version number.

This update is mainly targeted at scenarios when one updates a router - right after calling update, the existing router version that is deployed remains the same while the new version enters the pending state. Instead of having to query the status of the Router actively in order to retrieve the new router version id when it has finally been deployed successfully, the user can simply be assured that his/her Router instance has a newly updated version id when the waiting function has returned succesfully.

@deadlycoconuts deadlycoconuts requested a review from a team March 23, 2022 05:00
@deadlycoconuts deadlycoconuts self-assigned this Mar 23, 2022
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment: In the future, may be clearer to name the branch on the origin like sdk_v0.2.0-branch (just because we have differences in the app vs sdk numbering and it also follows the tag name format to prefix with 'sdk'). Anyway, we can nuke these branches when they've served their purpose.

sdk/turing/router/router.py Outdated Show resolved Hide resolved
sdk/turing/router/router.py Outdated Show resolved Hide resolved
openapi_docker_config = docker_config.copy()
openapi_docker_config['resource_request'] = \
turing.generated.models.ResourceRequest(**openapi_docker_config['resource_request'])
openapi_docker_config['env'] = [turing.generated.models.EnvVar(**env_var) for env_var in openapi_docker_config['env']]
Copy link
Contributor Author

@deadlycoconuts deadlycoconuts Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made some additional changes - they look pretty minor but they were causing some bugs in the unit tests after the refactoring suggested earlier.

Essentially, the previous implementation of the setter methods were mutating the dictionaries that were being passed to it as arguments, which is dangerous and might lead to unintended effects. These new changes simply create new working dictionaries that are only used within the scope of the setter.

cc @krithika369

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Thanks for addressing them. We need to propagate these changes to main later as well.

self._config = config
if self._config is not None and 'project_id' in self._config:
self.config['project_id'] = int(self._config['project_id'])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as the one I just commented on, though the fix here was to simply reorder the statements.

@deadlycoconuts
Copy link
Contributor Author

Okay thanks for your comments! I'll be merging this now.

@deadlycoconuts deadlycoconuts merged commit e7ef914 into caraml-dev:v0.2.1 Mar 23, 2022
@deadlycoconuts deadlycoconuts deleted the add_wait_for_status_update branch March 24, 2022 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants