-
Notifications
You must be signed in to change notification settings - Fork 54
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
Adding possibility to run the cli in the same context as the login action #145
base: master
Are you sure you want to change the base?
Conversation
Hi @wherka-ama, thank you for your contribution! Your suggestion to run azure/cli in the same context as azure/login does make sense. However, the azure/cli action is designed to support running multiple versions on an agent. Your proposed change would disable the Add our product manager @Jacekey23 here for awareness. |
Hi @MoChilia , thank you for your time spent on reviewing my proposal so promptly. Much appreciated :) If I understood your intention, instead of adding a new input parameter, we would simply check if the I'd be more than happy with this flow :) Cheers |
…t and in-container mode of execution
@MoChilia , @Jacekey23: just to let you know I've pushed a little tweak to make it aligned with your recommendation. I also tweaked the README to reflect it. Please do let me know if that's what you meant. BTW: I'm clarifying the CLA with my management. The moment I have a green light I'll sign it and will let you get on with the next steps. That's of course subject to our agreement that we are on the same page with this enhancement. Thanks again for your prompt response and openness. That's really appreciated ❤️ |
@wherka-ama, you correctly understand my intent, and thanks for your work on this. But this is actually a breaking change, we need to slow down and discuss within the team. I will keep you informed once we have reached a decision. |
Hi @MoChilia , sure :-) Let's try to have a proper think about it and do it cleanly - in a safe manner. Thanks again 👍 |
@MoChilia , @Jacekey23 : just checking in.. did you by any chance have a little chat on the subject? I would love to have your feedback and align with your chosen strategy. The last thing on my mind would be to fork this action, but I'm afraid that's what I'm using right now, and I really hate it :( |
Hey folks, |
@microsoft-github-policy-service agree |
@a-shtifanov-laya : I'm afraid I cannot give you any clue on that. |
Background
There is no doubt
azure/cli
action is very useful. There is no denying of this obvious fact.However, the way it has been designed was driven by assumptions which make it near impossible to use this action safely in the context of the self-hosted runners in Kubernetes e.g. under the supervision of the Action Runner Controller.
Furthermore it also imposes a strict requirement on having docker which in some cases may not be advisable or even possible in a more restricted environments where the only purpose of the job is to login to azure and run some commands.
Proposed solution
azure/cli
is very often linked withazure/login
. One cannot do without the other. This implies the need for havingaz
cli preinstalled in the runner context. It makes sense to allow leveraging this setup and invoke the commands directly rather than inside the container.I'm not suggesting a complete rewrite, but a mere addition of the input parameter to control this behaviour.
The default behaviour would remain the same i.e. running the command in container.
However, when the caller sets the
inContainer: false
, then we execute the generated script directly in the same context.This way we will not impact anyone who is used to current behaviour, but we can easily extend the consumer base. I bet there is a huge number of people who would love to see this feature implemented. I would not even mentioned the performance benefits stemming from the fact of skipping the creation of the extra container and in general making it safer.
Disclaimer
The code proposed here is very simple, yet effectively offers the functionality in scope. I'd be happy to work on it a bit more if the maintainers are interested in this direction. I can also offer it as a throw-away piece, which could be used as a basis for a cleaner option. Either way works for me. My intent is to make it work, not to take credit.
Side note: I'm not going to delve into details here(I can elaborate in priv if needs be), but currently the only option is to have a docker rootful daemon or a privileged container in the play. That comes with huge security risks. Running the az commands directly on the runners mitigates them successfully.
The PR fixes many issues across both
azure/cli
andazure/login
e.g. Azure/login#429