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(servstate): expand environment variables for service commands #402

Conversation

zhijie-yang
Copy link

@zhijie-yang zhijie-yang commented Mar 29, 2024

For reference, this fixes the command part of #182.

@zhijie-yang zhijie-yang marked this pull request as draft March 29, 2024 08:31
@benhoyt benhoyt changed the title fix (handlers): expand environment variables for service commands feat(servstate): expand environment variables for service commands Apr 1, 2024
@cjdcordeiro
Copy link
Collaborator

fyi, the RK020 spec is not about this specifically.

@benhoyt
Copy link
Contributor

benhoyt commented Apr 3, 2024

Hi @zhijie-yang, thanks for the submission. Out of interest, do you have a need for this as part of the RK020 work, or is this just something you want to fix separately? The reason I ask is I'm pressed for time at the moment and may not get a chance to review this properly for a couple of weeks.

In the meantime, at a quick glance it looks a bit convoluted, and there's a fair bit of manual, error-prone parsing going on with strings.TrimLeft etc. It looks like it only addresses the command part of the #182 (my comment further down), not the main issue with expanding $VAR in the environment itself. Is that your intention? It would be good to see a proper PR description with some examples of what this covers.

@zhijie-yang zhijie-yang closed this Apr 4, 2024
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.

3 participants