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

Separate out the hosting CLI from main repo #2165

Merged
merged 20 commits into from
Nov 28, 2023
Merged

Conversation

martinxu9
Copy link
Contributor

@martinxu9 martinxu9 commented Nov 13, 2023

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

After these steps, you're ready to open a pull request.

  • Separate out the hosting CLI logic into its own repo (https://github.com/reflex-dev/reflex-hosting-cli/pull/1), this PR remove the functions that are moved. Including the unit tests.
  • reflex-hosting-cli becomes a dependency of reflex, and make it >= so it'll be easy to update hosting CLI without needing to upgrade reflex framework.
  • Remove dependencies added by hosting CLI over time: websockets, tabulate.
  • Refactor reflex export command implementation by moving the underly function into its own file utils/export.py. This way the export util can be passed into hosting CLI as callback. Deploy command executes export in the middle of it. This part is less ideal, the flow of code runs back and forth. Ideally with hosting CLI as dependency, the flow should be from reflex to hosting CLI.
  • Tested with the pre-release host CLI v0.0.1a: deploy, list, logs, build-logs and delete.

@martinxu9 martinxu9 marked this pull request as draft November 13, 2023 23:34
@martinxu9 martinxu9 changed the title Separate out the hosting CLI from main repo Separate out the hosting CLI from main repo [DO NOT MERGE] Nov 13, 2023
config = get_config()


def export(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the main logic inside the export typer command, rid it out into its own function so it can be passed into the hosting CLI as "callback" to run export.

# Set the log level.
console.set_log_level(loglevel)

# Override the config url values if provided.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is added logic. Since hosting CLI does not have access to config, it can only pass back these for the reflex framework to override its config.

"Please provide a name for the deployed instance when not in interactive mode."
)
raise typer.Exit(1)

dependency.check_requirements()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are reordered to run first (they mostly should be anyway), so don't have to pass any functions into the hosting CLI.

@martinxu9 martinxu9 requested review from Alek99 and picklelo November 18, 2023 00:10
@martinxu9 martinxu9 marked this pull request as ready for review November 18, 2023 00:45
@Alek99
Copy link
Member

Alek99 commented Nov 20, 2023

Seems to be working for me just tried a deployment on the branch

@martinxu9 martinxu9 changed the title Separate out the hosting CLI from main repo [DO NOT MERGE] Separate out the hosting CLI from main repo Nov 20, 2023
@martinxu9 martinxu9 force-pushed the martinxu9/separate-cli branch from 732d818 to bd96790 Compare November 21, 2023 09:09
@martinxu9
Copy link
Contributor Author

This PR is ready.

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Code looks good - just wondering if the initial requirements.txt logic creation should be moved back to this repo

reflex/reflex.py Outdated
from reflex.utils import console, dependency, telemetry
from reflex.utils import (
console,
telemetry,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can remove the last comma to fit this on one line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

reflex/reflex.py Outdated
@@ -103,7 +101,7 @@ def _init(
prerequisites.initialize_gitignore()

# Initialize the requirements.txt.
prerequisites.initialize_requirements_txt()
dependency.initialize_requirements_txt()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be part of this package since it's a bit unrelated to the hosting? The initial one will only have the reflex package in it anyways. If we want to update this it may make it harder

Copy link
Contributor Author

@martinxu9 martinxu9 Nov 28, 2023

Choose a reason for hiding this comment

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

Synced on discord. The desired code is in here now: https://discord.com/channels/@me/1141437177448906882/1179140841089007700, we'll move that back to reflex repo so open source can see what is actually done when we init.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in the latest rev

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Tested a deploy and worked well for me

@picklelo picklelo merged commit f8395b1 into main Nov 28, 2023
45 checks passed
@martinxu9 martinxu9 deleted the martinxu9/separate-cli branch November 28, 2023 23:54
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