Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

Fix flyio org option not working #662

Merged
merged 4 commits into from
May 10, 2023
Merged

Fix flyio org option not working #662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dc3888d
Fix flyio org option not working
aminalaee Apr 26, 2023
6443f9b
Edit docstring
aguschin Apr 27, 2023
af83bae
add test
aminalaee May 1, 2023
054ca10
install flyctl
aminalaee May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/check-test-release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ jobs:
env:
HEROKU_API_KEY: ${{ secrets.HEROKU_API_KEY }}
HEROKU_TEAM: iterative-sandbox
- name: Run Flyctl tests
if: matrix.os == 'ubuntu-latest' && matrix.python == '3.9'
run: |
curl -L https://fly.io/install.sh | sh
pytest -k 'flyctl'
- name: Start minikube
if: matrix.os == 'ubuntu-latest' && matrix.python == '3.9'
uses: medyagh/setup-minikube@master
Expand Down
4 changes: 3 additions & 1 deletion mlem/contrib/flyio/meta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class Config:

class FlyioSettings(BaseModel):
org: Optional[str] = None
"""Organization name"""
"""Organization slug (run `flyctl orgs list` to find out the right one)"""
aminalaee marked this conversation as resolved.
Show resolved Hide resolved
region: Optional[str] = None
"""Region name"""

Expand Down Expand Up @@ -110,6 +110,8 @@ def _create_app(self, state: FlyioAppState):
args["generate-name"] = True
if self.get_env().access_token:
args["access-token"] = self.get_env().access_token
if self.org:
args["org"] = self.org
Copy link
Contributor Author

@aminalaee aminalaee Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I couldn't find any tests for flyio

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, looks like we didn't add them 👀

Copy link
Contributor

@aguschin aguschin Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this something you might be interested in? As a first step I think we should add a test for _create_app, asserting that resulting fly.toml looks right. This test can also help us pin down the root cause of #657

The second step would be to add a real e2e deploy to fly. I can create a test account there and set up GHA workflow to make it work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sounds good. Do you want to add the tests in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this PR is the right place :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aguschin Any idea how this can be tested?
I'm doing

def test_create_app():
    flyio_app = FlyioApp()
    state = flyio_app.get_state()
    flyio_app._create_app(state)

But the get_state() fails because of mlem.core.errors.MlemObjectNotSavedError: Not saved object has no location.
even if I create the app with a default state, while updating the state the issue happens again. I think I'm missing a step before the the call to create_app.

Copy link
Contributor

@aguschin aguschin May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @aminalaee! Sure, let me help you out 🙌🏻
It needs to be dumped first:

from mlem.contrib.flyio.meta import FlyioApp


def test_create_app(tmpdir):
    flyio_app = FlyioApp()
    flyio_app.dump(str(tmpdir))
    state = flyio_app.get_state()
    flyio_app._create_app(state)

If you run this test, it will fail still (on the last line though), because creating flyio app requires Dockerfile present (...Could not find a Dockerfile, nor detect a runtime or framework from source code. Continuing with a blank app.\x1b).

Copy link
Contributor

Choose a reason for hiding this comment

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

You either need to bypass this, or maybe call mlem.api.deploy directly on flyio_app while mocking some actual deployment mechanisms. Then, until the temp dir with fly.toml and stuff is removed, you can read it in the test to make some checks. Hope that makes it clearer! If confused, please ping me, I'll help with that.

run_flyctl("launch", workdir=tempdir, kwargs=args)
state.fly_toml = read_fly_toml(tempdir)
port = getattr(self.server, "port", None) or getattr(
Expand Down
34 changes: 34 additions & 0 deletions tests/contrib/test_flyio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
from pathlib import Path
from unittest.mock import ANY, patch

from mlem.contrib.flyio.meta import FlyioApp
from mlem.contrib.flyio.utils import FlyioStatusModel


def test_create_app(tmp_path: Path):
flyio_app = FlyioApp(org="org", app_name="test")
flyio_app.dump(str(tmp_path))
state = flyio_app.get_state()
status = FlyioStatusModel(
Name="test", Hostname="fly.io", Status="Deployed"
)

with patch("mlem.contrib.flyio.meta.run_flyctl") as run_flyctl:
with patch("mlem.contrib.flyio.meta.read_fly_toml") as read_fly_toml:
with patch("mlem.contrib.flyio.meta.get_status") as get_status:
with patch("mlem.contrib.flyio.meta.FlyioApp._build_in_dir"):
get_status.return_value = status
read_fly_toml.return_value = ""
flyio_app.deploy(state)

run_flyctl.assert_called_once_with(
"launch",
workdir=ANY,
kwargs={
"auto-confirm": True,
"region": "lax",
"no-deploy": True,
"name": "test",
"org": "org",
},
)