-
Notifications
You must be signed in to change notification settings - Fork 243
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
Inventing deploy manifest #3769
Conversation
d590082
to
912d7ec
Compare
internal/command/deploy/deploy.go
Outdated
@@ -215,6 +215,19 @@ func New() *Command { | |||
Description: "Do not run the release command during deployment.", | |||
Default: false, | |||
}, | |||
flag.Bool{ | |||
Name: "remote-deploy", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rugwirobaker ! Here's trying my first pass on code reviewing a flyctl-deployments-related PR( let me know if this kind of review is useful or not! ):
Topic: I want to point out, in case this "remote-deploy" flag can be commented out, implemented, or of course, left as-is for future use
-
Is the flag "remote-deploy" already used when running the deploy command? The only code section I can find it mentioned in is this commented out line. Which points to the use of the function deployRemotely which I can't find declared anywhere just yet
-
If there's no implementation yet for its use, does this flag need to be functional before this PR is merged? It's not right, as this PR will just "provide the basis for remote deployers..."?
-
If it's not yet needed, can this be commented out for now? I think this can be added later on a separate PR, with an the implementation for its use.
-
But! Of course, it can also be left alone, as it is now, for future use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I commented it out that part but forgot to remove the flag. Thanks for pointing it out.
test/preflight/fly_deploy_test.go
Outdated
appName := f.CreateRandomAppName() | ||
f.Fly("launch --org %s --name %s --region %s --image nginx --internal-port 80 --ha=false", f.OrgSlug(), appName, f.PrimaryRegion()) | ||
|
||
f.Fly("deploy --export-manifest manifest.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also manually tested the two commands above!
-
deploy --export-manifest manifest.json creates a manifest.json file:
Running
flyctl.exe deploy --export-manifest manifest.json
, provides the following relevant results:
> The "Deploy manifest saved to manifest.json" log was printed
> Created a manifest.json file in the root directory of my Laravel Fly App project -
deploy --manifest manifest.json, will deploy using the manifest.json file, and reflect changes to the machine
Now the fun part, to see if the next command works( which, I think can modify the deployment process right?), I added a new environment variable in manifest.json:
"env": { "APP_ENV": "production", "DB_CONNECTION": "pgsql", "LOG_CHANNEL": "stderr", "LOG_LEVEL": "info", "LOG_STDERR_FORMATTER": "Monolog\\Formatter\\JsonFormatter", "SESSION_DRIVER": "cookie", "SESSION_SECURE_COOKIE": "true", + "test": "test" },
Then I run the command
flyctl.exe deploy --manifest manifest.json
. Afterwards, I connected with the machine withfly ssh console
, checked if that env variable was reflected, and it was!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KTanAug21 Yeah, that's exactly it. The manifest can be sent to a remote machine that can resume the deployment without uploading the whole codebase. I hadn't thought that you could modify the manifest post-generating it. This makes me wonder if that's desirable or if we'd want to make it immutable by generating some signature that will tell us if it was modified after it was generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! Is this behavior of the manifest "being modifiable" the reason, why you've added that case "-" for getting the manifest from io.In
?.
Will getting the manifest stream from io.In make sure that the manifest content wasn't modified?
this is excellent work btw. this is a really great solution for remote deploys |
}, | ||
flag.String{ | ||
Name: "manifest", | ||
Description: "Path to a deploy manifest file to use for deployment.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it also worth mentioning in the Description the use of "-" for getting manifest from io.In, as can be seen in this case "-"?
Also! I'm curious! I can't imagine how the scenario for using "-" actually works( I want to test it out). Is it like this?
- run
fly deploy --export-manifest="-"
, will print manifest details to stdout - run
fly deploy --manifest="-"
from ( somewhere else(a different console or machine?) OR the same console/machien? ) to get the manifest stream from running step Command to generate manifest #1 above, so that it can catch its stdout stream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is it something like run fly deploy --export-manifest="-"
as input to fly deploy --manifest="-"
?
If that's it, then how can I run it from my local console?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up I changed --manifest
to --from-manifest
. I think fly deploy --from-manifest
is better equipped to convey that we are deploying using a manifest.
And the right format is fly deploy --from-manifest -
and fly deploy --export-manifest
. This is mostly because flag.GetString always returns ""
even when the flag is not specified. The if flag.GetString(ctx, "from-manifest")=="" would always be true and take over everything if we decided that an absence of the manifest filename means we will be reading from StdIn. So I decided to use this pattern cause I saw it used inf
kubectl`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can still improve the docs though
46a100a
to
757405b
Compare
Change Summary
What and Why:
We need a way to start a deployment in one session on one machine and finish it in a different session or even another machine.
This will form the basis for our remote deployers that will help us monitor and recover from failed deployments without user intervention.
How:
To do that I'm investing what I will call a "deploy manifest" is basically a json blob/file that contains a json version of the fly.toml config file
the different flags we've passed to
fly deploy
and an image, the result of the build process. We're basically hijacking the deploy process just afterwe've figured out all the deployment options and exporting them with the
--export-manifest flag
. To resume the rest of the deploy process we willpass it to
fly deploy
with a--manifest
flag.Related to:
N/A
Documentation