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.

Sign up for GitHub

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

Jump to bottom

[rqd] feat: add new config option RQD_USE_PATH_ENV_VAR #1241

Merged
merged 3 commits into from
Jan 27, 2023
Merged

[rqd] feat: add new config option RQD_USE_PATH_ENV_VAR #1241

merged 3 commits into from
Jan 27, 2023

Conversation

romainf-ubi
Copy link
Contributor

Link the Issue(s) this Pull Request is related to.
Fixes #1240

Summarize your change.
This option overrides the hardcoded PATH env var defined in rqd/rqd/rqmachine.py, in the function getEnvPath(), line 508.

It allows to use the PATH environment variable defined on the RQD host.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 12, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@bcipriano
Copy link
Collaborator

@romainf-ubi Regarding the CLA check here -- the EasyCLA comment lists @Damien-G. Probably the commit metadata has their name/email attached? So I believe either they would need to do the "click here to be authorized" step or the commits could be modified with your info.

@bcipriano
Copy link
Collaborator

Regarding the other test failures:

  1. Python unit tests passed, but it's showing as failed due to an xvfb-run crash. That should be hopefully fixed by Add wrapper script for cuegui tests. #1236, but you can ignore it for now.
  2. The lint check is accurate -- it has flagged a (very) minor issue.

@romainf-ubi
Copy link
Contributor Author

@bcipriano I fixed the CLA issue.

bcipriano
bcipriano approved these changes Jan 23, 2023
View reviewed changes
Copy link
Collaborator

@bcipriano bcipriano left a comment

Choose a reason for hiding this comment

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

Change LGTM, thanks for doing this!

Just a couple minor comments, and you'll need to do one more merge from master to resolve those other PR checks.

rqd/rqd/__main__.py Outdated Show resolved Hide resolved
rqd/rqd/rqconstants.py Show resolved Hide resolved
@romainf-ubi
[rqd] feat: add new config option RQD_USE_PATH_ENV_VAR
b40270f 
This option overrides the hardcoded PATH env var defined in
rqd/rqd/rqmachine.py, in the function `getEnvPath()`, line 508.

It allows to use the PATH environment variable defined on the RQD host.
@romainf-ubi
fix(linter): line was too long, used a multiline f-string
1998966
@romainf-ubi
fix: add comment for the new conf option
fe730d0
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 23, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@romainf-ubi
Copy link
Contributor Author

@bcipriano everything is fixed (you can resolve the threads if you're ok with the chosen solutions), and I rebased my branch on the latest master as well.

@bcipriano bcipriano merged commit bb74558 into AcademySoftwareFoundation:master Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcipriano bcipriano bcipriano approved these changes

@gregdenton gregdenton Awaiting requested review from gregdenton

@jrray jrray Awaiting requested review from jrray

@smith1511 smith1511 Awaiting requested review from smith1511

@larsbijl larsbijl Awaiting requested review from larsbijl

@DiegoTavares DiegoTavares Awaiting requested review from DiegoTavares DiegoTavares is a code owner

@IdrisMiles IdrisMiles Awaiting requested review from IdrisMiles

@splhack splhack Awaiting requested review from splhack

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Being able to use RQD's PATH env var
2 participants
@romainf-ubi @bcipriano