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

Allow passing extra arguments to eval release command #12292

Merged

Conversation

hauleth
Copy link
Contributor

@hauleth hauleth commented Dec 7, 2022

It is handy for writing custom actions to communicate and operate with the release.

@hauleth hauleth force-pushed the ft/allow-passing-extra-args-to-release branch from 1426543 to f58dcfd Compare December 7, 2022 11:43
@josevalim
Copy link
Member

@wojtekmach, I assume we discussed this at some point but I can't quite recall. Or did we run into issues with RPC?

@hauleth we would also need to support this on mix eval and add a test. :)

@hauleth
Copy link
Contributor Author

hauleth commented Dec 7, 2022

At first I added it to mix eval as well, but I wasn't sure it will work. First I need to make it work on Windows, but I know nothing about Batch so I am testing how I can make it work.

@josevalim
Copy link
Member

@hauleth I believe you can change System.argv on the fly. Or perhaps see how mix run does it.

@wojtekmach
Copy link
Member

@josevalim I don't think we ever tackled bin/app eval. We were mostly concerned about bin/app start (#11426 (comment)) and a new bin/app rpc MOD FUN ...ARGS (#11661)

@hauleth hauleth force-pushed the ft/allow-passing-extra-args-to-release branch from f58dcfd to 9a0513a Compare December 7, 2022 13:10
Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

LGTM! We just need to address mix eval and we are good to go.

@hauleth hauleth force-pushed the ft/allow-passing-extra-args-to-release branch from 9a0513a to 2d56df9 Compare December 8, 2022 14:01
@hauleth
Copy link
Contributor Author

hauleth commented Dec 8, 2022

Ehhhh… Do we need to support Batch? Cannot we support PowerShell on Windows instead?

https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/shift

The shift command has no effect on the %* batch parameter.

Ok, so looping, here we go.

@hauleth hauleth force-pushed the ft/allow-passing-extra-args-to-release branch from 2d56df9 to 03a6ddd Compare December 8, 2022 14:27
@hauleth hauleth marked this pull request as draft December 8, 2022 14:29
@wojtekmach
Copy link
Member

wojtekmach commented Dec 8, 2022

We can’t default to .ps1 as executing these is disabled by default on Windows. (ikr.)

@hauleth hauleth force-pushed the ft/allow-passing-extra-args-to-release branch 3 times, most recently from af0192c to 5873466 Compare December 8, 2022 15:49
@hauleth hauleth force-pushed the ft/allow-passing-extra-args-to-release branch from 5873466 to 54a0817 Compare December 8, 2022 16:23
@hauleth hauleth marked this pull request as ready for review December 8, 2022 16:23
@hauleth hauleth force-pushed the ft/allow-passing-extra-args-to-release branch from 0f586be to 54a0817 Compare December 9, 2022 14:17
@hauleth
Copy link
Contributor Author

hauleth commented Dec 9, 2022

@josevalim eval is done, the rpc is not, but honestly I do not know if that is something we should do. The thing is that rpc is called in context of the application, so that mean that we would not be able to do rpc "IO.inspect(System.argv())" to get the args that were passed to start command. I think that eval is the only one where it is sensible to have access to passed args. For rpc there would be another mechanism needed for that.

@josevalim
Copy link
Member

Yup. We cannot do rpc.

@hauleth
Copy link
Contributor Author

hauleth commented Dec 9, 2022

So now it is ready, works on Windows (at least tests says so) and on *nixes.

@josevalim
Copy link
Member

Thank you @hauleth! We just need to support the same on the new mix eval task and we are good!

@hauleth
Copy link
Contributor Author

hauleth commented Dec 9, 2022

There is new command for mix beyond mix run? Ok, I will take a look.

@josevalim
Copy link
Member

mix eval was designed to mirror the VM state from bin/app eval to make it easier for testing and reuse!

@hauleth
Copy link
Contributor Author

hauleth commented Dec 13, 2022

Added the support for arguments in mix eval

@hauleth hauleth force-pushed the ft/allow-passing-extra-args-to-release branch from 947f835 to 324e113 Compare December 13, 2022 16:13
@josevalim josevalim merged commit b36ed81 into elixir-lang:main Dec 13, 2022
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@hauleth hauleth deleted the ft/allow-passing-extra-args-to-release branch January 13, 2023 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants