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

Remove token check in reflex deploy #4640

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

ElijahAhianyo
Copy link
Contributor

@ElijahAhianyo ElijahAhianyo commented Jan 15, 2025

This logic has been moved upstream to reflex-hosting-cli (via https://github.com/reflex-dev/reflex-hosting-cli/pull/102)

This logic has been moved upstream to reflex-hosting-cli
@Lendemor
Copy link
Collaborator

Going to mark this as draft until the PR for the hosting-cli is merged.

Also maybe we want the poetry lock update in the same PR, for the requirements to the next version of hosting-cli.

@Lendemor Lendemor marked this pull request as draft January 15, 2025 16:13
@ElijahAhianyo ElijahAhianyo marked this pull request as ready for review January 15, 2025 19:25
@ElijahAhianyo
Copy link
Contributor Author

ElijahAhianyo commented Jan 15, 2025

Going to mark this as draft until the PR for the hosting-cli is merged.

Also maybe we want the poetry lock update in the same PR, for the requirements to the next version of hosting-cli.

The requirements for that is already bound (reflex-hosting-cli = ">=0.1.29,<2.0"), shouldn't that be automatically picked up in the next reflex release if the CLI gets released?

@Lendemor
Copy link
Collaborator

Lendemor commented Jan 15, 2025

Going to mark this as draft until the PR for the hosting-cli is merged.
Also maybe we want the poetry lock update in the same PR, for the requirements to the next version of hosting-cli.

The requirements for that is already bound (reflex-hosting-cli = ">=0.1.29,<2.0"), shouldn't that be automatically picked up in the next reflex release if the CLI gets released?

Next version won't work with <=0.1.32, so we need to update the lower bound anyway.

@masenf
Copy link
Collaborator

masenf commented Jan 15, 2025

if the lower bound isn't raised, then upgrades of reflex won't install the new reflex-hosting-cli

that said, it's kind of a wash anyway because we have a secondary runtime check for latest version of hosting-cli and tell people to upgrade manually then

@ElijahAhianyo
Copy link
Contributor Author

if the lower bound isn't raised, then upgrades of reflex won't install the new reflex-hosting-cli

that said, it's kind of a wash anyway because we have a secondary runtime check for latest version of hosting-cli and tell people to upgrade manually then

In that case what's the best option here? Allow reflex upgrade to handle upgrading the CLI or leave it to users to do that? The former seems plausible but I'm wondering if there are any repercussions that would make us settle for the latter

@masenf
Copy link
Collaborator

masenf commented Jan 17, 2025

i think it's better to bump the lower bound if this PR depends on things in a later version of reflex-hosting-cli

@masenf masenf merged commit 0484161 into main Jan 21, 2025
41 checks passed
@masenf masenf deleted the elijah/remove-deploy-token-check branch January 21, 2025 21:28
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.

3 participants