-
Notifications
You must be signed in to change notification settings - Fork 13
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
Option to force build before deploy #97
Conversation
#[clap(long, takes_value = false, env = "SPIN_ALWAYS_BUILD")] | ||
pub build: bool, |
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 could potentially add a shorter argument "-b"? Just curious and do not have any strong preference or is this match with spin
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.
spin up
currently only supports the long form.
src/commands/deploy.rs
Outdated
|
||
async fn run_spin_build(&self) -> Result<()> { | ||
let manifest_path = self.app()?; | ||
let spin_bin = std::env::var("SPIN_BIN_PATH")?; |
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.
Would it make sense to add this as a constant? I mean the string "SPIN_BIN_PATH"
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 tried creating a const but it ended up feeling like noise so I rage-encapsulated it as a "give me the Spin binary path" function instead
src/commands/deploy.rs
Outdated
/// Specifies to perform `spin build` before deploying the application. | ||
#[clap(long, takes_value = false, env = "SPIN_ALWAYS_BUILD")] | ||
pub build: bool, | ||
|
||
/// Deploy existing bindle if it already exists on bindle server | ||
#[clap(short = 'e', long = "deploy-existing-bindle")] |
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.
Does the build option conflict with deploy existing bindle? How should one think about these two options?
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.
Does the deploy existing bindle option still exist? I couldn't see it in the code but yeah there's not much point doing a build of the local app just to deploy an existing remote app (although I think it would be harmless but I'm not sure what deploy existing bindle does).
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.
Oh I see it existed in main when I wrote this PR but not when I looked at main just now. So I think this conundrum has gone away (thanks Vaughn and Team OCI)
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.
Not sure. It would be nice to sync on all the bindle related code for 2.0 (added to 2.0 board). For now, does it make sense to add a conflicts_with
requirement to the deploy-existing-bindle
flag for clarity? Agree on it being harmless though.
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.
Sorry - I wasn't clear because I was figuring things out as I went! I can't add a conflicts_with
on deploy-existing-bindle
because deploy-existing-bindle
was removed. There is no bindle any more: there is only OCI. And there is no longer a redeploy option. So the problem goes away.
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.
excellent!
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
137fba2
to
d942023
Compare
Fixes #95.
Happy path:
Sad path:
I know that's a lot of errors at the end but that's fiddly to fix with the way Rust handles
Result::Err
exits. It can be addressed but I'm not keen to rework that as part of this PR.