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

Feature: RQD command expand env vars #1323

Conversation

Tilix4
Copy link

@Tilix4 Tilix4 commented Oct 20, 2023

Link the Issue(s) this Pull Request is related to.
Started discussion in discussion mail list.

Summarize your change.
Run os.path.expandvars before the command is executed on RQD host. This allows to use Environment Variables, for path resolving it might be a great help.

I haven't found any place in documentation where to accurately put this information. Please tell me.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 20, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Tilix4 / name: Félix David (72cee3b)

Copy link
Collaborator

@DiegoTavares DiegoTavares left a comment

Choose a reason for hiding this comment

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

I don't see a problem in expanding env vars, but since this problem seems to be windows specific, to avoid messing with the way linux hosts work, I would suggest making this command windows specific. (move this call to line 141)

@Tilix4
Copy link
Author

Tilix4 commented Nov 9, 2023

I don't see a problem in expanding env vars, but since this problem seems to be windows specific, to avoid messing with the way linux hosts work, I would suggest making this command windows specific. (move this call to line 141)

How would it mess with how linux hosts work? I don't get where the issue would come from, but I might be missing something.

@bcipriano
Copy link
Collaborator

I think the concern is that the RQD environment could differ from the environment in which the frame command is running. Users have all sorts of funky setups using wrapper scripts and similar, and may not be expecting those env vars to be expanded prior to the command being executed. So this change could be disruptive to them.

If I understand correctly, Windows hosts don't currently expand env vars in the command because of the way execution works on Windows, with commands being wrapped in a temporary batch file. So this PR is unlikely to break anything for Windows users.

@DiegoTavares
Copy link
Collaborator

@angelali-ms Are you still interested in working on this PR?

@angelali-ms
Copy link
Contributor

@angelali-ms Are you still interested in working on this PR?

Hi @DiegoTavares :) I think I mainly mentioned this PR in another issue because it seemed related. That issue has been resolved now.
I believe the original PR was created by @Tilix4!

@DiegoTavares
Copy link
Collaborator

@angelali-ms Are you still interested in working on this PR?

Hi @DiegoTavares :) I think I mainly mentioned this PR in another issue because it seemed related. That issue has been resolved now. I believe the original PR was created by @Tilix4!

🤦🏻‍♂️ Sorry, I simply assumed the last comment was the PR owner. My bad.

@Tilix4 is your work on this PR still ongoing?

@DiegoTavares
Copy link
Collaborator

Closing this PR due to inactivity.

This feature is desirable, but it would need to be configurable as not all environment want local environments to be expanded.

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