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

feat!: Handle 0.7 breaking changes and updates [SBK-369] #46

Merged
merged 10 commits into from
Jan 2, 2024

Conversation

NotPeopling2day
Copy link
Contributor

What I did

fixes: #

How I did it

How to verify it

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

@vany365 vany365 changed the title feat!: update versions feat!: update versions [SBK-369] Nov 22, 2023
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
silverback/settings.py Outdated Show resolved Hide resolved
.mdformat.toml Outdated Show resolved Hide resolved
@NotPeopling2day NotPeopling2day changed the title feat!: update versions [SBK-369] feat!: Handle 0.7 breaking changes and updates [SBK-369] Dec 18, 2023
fubuloubu
fubuloubu previously approved these changes Dec 20, 2023
@fubuloubu fubuloubu requested review from mikeshultz and antazoey and removed request for antazoey December 20, 2023 20:24
@fubuloubu
Copy link
Member

@antazoey if you can check the updates I made to CLI
@mikeshultz if you can make sure I didn't break anything you recently changed with persistence or the workers

Copy link
Contributor

@mikeshultz mikeshultz left a comment

Choose a reason for hiding this comment

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

Couple questions inline. Generally looks good. Has this been tested? I'm especially curious if anything broke with the taskiq upgrade.

setup.py Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
@fubuloubu
Copy link
Member

Couple questions inline. Generally looks good. Has this been tested? I'm especially curious if anything broke with the taskiq upgrade.

I ran the in-memory broker example from the README

Would need more testing with some other usecases, specifically with the worker and with persistence

mikeshultz
mikeshultz previously approved these changes Dec 20, 2023
Copy link
Contributor

@mikeshultz mikeshultz left a comment

Choose a reason for hiding this comment

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

Tested locally. LGTM

Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

I think default=None is no longer needed on the @network_option. It doesn't work with ConnectedProviderCommand anyway, as it will use the default network in that case, which is the same as not specifying default=None.

silverback/_cli.py Outdated Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
@fubuloubu fubuloubu enabled auto-merge (squash) January 2, 2024 20:19
@fubuloubu fubuloubu disabled auto-merge January 2, 2024 20:21
os.environ["SILVERBACK_NETWORK_CHOICE"] = val
# NOTE: Make sure both of these have the same setting
if env_network_choice := os.environ.get("SILVERBACK_NETWORK_CHOICE"):
if val.network_choice != env_network_choice:
Copy link
Member

Choose a reason for hiding this comment

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

Note: @mikeshultz noticed if you use the shortened form in the environment variable, that the selection would be fully resolved and thus be mismatching

easy fix is to just update the environment variable to the fully resolved form

@fubuloubu fubuloubu merged commit 1092046 into ApeWorX:main Jan 2, 2024
13 checks passed
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.

4 participants