Fix flyio org option not working #662
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
I couldn't find any tests for flyio
There was a problem hiding this comment.
Good point, looks like we didn't add them 👀
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah sounds good. Do you want to add the tests in a separate PR?
There was a problem hiding this comment.
No, this PR is the right place :)
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #662 +/- ##
=======================================
Coverage 85.75% 85.76%
=======================================
Files 109 109
Lines 9922 9924 +2
=======================================
+ Hits 8509 8511 +2
Misses 1413 1413
☔ View full report in Codecov by Sentry. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Hi @aminalaee! Overall, LGTM! Two comments:
|
|
I will update the actions to install the flyctl cli. but this limits us for number 2 which adds an integration test. We can add another one. |
|
Hi @aminalaee! How are you doing? Is everything ok? I tried to reach you out by email btw, but didn't get any answer. |
|
Any help needed with this? |
|
Hey @aguschin I was thinking to fix the pipeline in this PR with the current mocked tests and setup the integration tests in a separate PR? |
|
Sure, this will work @aminalaee. Thanks! |
Subset of #661