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

feat: add --features argument to cargo shuttle run #914

Closed

Conversation

orhun
Copy link
Contributor

@orhun orhun commented May 13, 2023

Description of change

This PR adds --features argument to run subcommand for activating cargo features.

See #913 for more detail.

How Has This Been Tested (if applicable)?

I ran it locally, worked fine.

@jonaro00
Copy link
Member

It's a good feature request.
It would be suitable to make it work for deployments as well. Not sure if this complicates things.
It's worth considering how this affects #756, but it seems completely fine.

@orhun
Copy link
Contributor Author

orhun commented May 13, 2023

It would be suitable to make it work for deployments as well. Not sure if this complicates things.

I was going to implement it for deployments as well but couldn't figure out how deploy command works. I guess the build happens somewhere else since it only sends a request along with a compressed archive, right? I would be happy to go ahead and implement it if I can get a better understanding of the internals.

It's worth considering how this affects #756, but it seems completely fine.

Oh, I think it is easily convertable to execute the cargo command instead, so I agree.

@jonaro00
Copy link
Member

It would probably work to add them as a URL parameter, like no_test is now.

But I remembered there is another challenge:
In the PR now, all shuttle crates in the workspace gets the same feature flags set.
Specifying flags to individual crates becomes a challenge.
Related:

@iamwacko
Copy link
Contributor

iamwacko commented Jun 6, 2023

#756 has been merged now, so you will have to make some changes.

@oddgrd
Copy link
Contributor

oddgrd commented Jun 8, 2023

Hey @orhun, thanks for getting started on this! Apologies for the delayed review, but we're in the middle of a major refactor of how we build crates and deploy users projects. I think it makes sense to hold off on this until that is all merged. The PR for the builder should be up within a few days!

@orhun orhun force-pushed the feat/run_subcommand_with_features branch from 4d8d93b to ca7a652 Compare June 23, 2023 21:45
@orhun
Copy link
Contributor Author

orhun commented Jun 23, 2023

Refactored the code to use the new cargo build mechanism. Let me know if it looks okay!

I might provide another PR for the deploy subcommand.

@jonaro00
Copy link
Member

There was a discussion about this the other day, regarding design of multi-service deploys.
Sadly, it looks like shuttle services will require the default binary and features to be the shuttle version of the crate.
It does not only apply to --features, so supporting customization of this sort becomes a catch up game with cargo's build and run

@orhun
Copy link
Contributor Author

orhun commented Jun 24, 2023

I see, in that case, what exactly is needed?

@jonaro00
Copy link
Member

@orhun
Copy link
Contributor Author

orhun commented Jun 24, 2023

Unfortunately I'm still not following. I guess I will wait a bit until things stabilize 😅

@jonaro00
Copy link
Member

Yeah, with the major deployment rewrite coming up there are a lot of question marks from me as well.

@oddgrd
Copy link
Contributor

oddgrd commented Sep 4, 2023

I'll close this for now @orhun, we can pick it up again when we're implementing the builder, but it will likely take a different shape.

@oddgrd oddgrd closed this Sep 4, 2023
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.

4 participants