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

W-17092332: Fix for updating functions without is_public set #650

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

jmoens
Copy link
Collaborator

@jmoens jmoens commented Oct 30, 2024

update_endpoint_info, and deploy with override set to True were both broken when trying to update a function that had been deployed before is_public existed.

The update code searched for the is_public attribute, and failed when it did not exist. To fix this, when updating a function we first check that is_public exist, and if it does not we default it False if no specific value is set in deploy or update.

@pep8speaks
Copy link

pep8speaks commented Oct 30, 2024

Hello @jmoens! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-10-31 20:23:57 UTC

current_is_public = False
if hasattr(endpoint_info, "is_public"):
current_is_public = endpoint_info["is_public"]
is_public = self._check_and_set_is_public(is_public, current_is_public)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks great! Another option with getattr:

is_public = self._check_and_set_is_public(is_public, getattr(endpoint_info, "is_public", False))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like this. Much neater.

@jmoens jmoens merged commit cde3b0d into master Oct 31, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants