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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass environment to sudo and fix tests #55

Merged
merged 3 commits into from
Dec 6, 2021
Merged

Pass environment to sudo and fix tests #55

merged 3 commits into from
Dec 6, 2021

Conversation

0x2b3bfa0
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 commented Dec 6, 2021 •
edited

May close #53; fixes bugs in the testing workflow. 馃悰馃敤

@0x2b3bfa0 0x2b3bfa0 requested a review from a team December 6, 2021 11:28
@0x2b3bfa0 0x2b3bfa0 self-assigned this Dec 6, 2021
@0x2b3bfa0 0x2b3bfa0 added the p0-critical bugs & errors label Dec 6, 2021
casperdcl
casperdcl previously approved these changes Dec 6, 2021
View reviewed changes
@0x2b3bfa0
Pass environment to sudo
58e05d2 
Add a missing semicolon

Hide and seek champion since 1958
@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Dec 6, 2021 •
edited

It turns out that sudo -E won't transfer PATH on most modern systems for the sake of security. Using https://unix.stackexchange.com/a/83194 instead...

@0x2b3bfa0 0x2b3bfa0 requested review from casperdcl and removed request for casperdcl December 6, 2021 11:48
@0x2b3bfa0
Copy link
Member Author

Tested and working as expected.

@0x2b3bfa0

This comment has been minimized.

Sign in to view
casperdcl
casperdcl previously approved these changes Dec 6, 2021
View reviewed changes
@casperdcl
Copy link
Contributor

casperdcl commented Dec 6, 2021 •
edited

did we find a bug in the tests?

EDIT: no, cml publish should accept stdin in lieu of a filename: https://github.com/iterative/cml/blob/cf7bfd039bc366d781ee897f774ca394f21e3f39/bin/cml/publish.js#L10

@0x2b3bfa0
Fix tests
f5c5132 
Update check.yml
@0x2b3bfa0
Copy link
Member Author

Yes, cml publish should and does accept stdin in lieu of a path, mais... vous trompez: tests actually had a bug, because they were running cml publish without a path and without stdin

@0x2b3bfa0
Copy link
Member Author

Get prepared to give away your soul for the third time in a row.

@0x2b3bfa0 0x2b3bfa0 mentioned this pull request Dec 6, 2021
Merged
@0x2b3bfa0
Fix tests on Windows
c3e62ff 
Legacy links aren't compatible
@0x2b3bfa0 0x2b3bfa0 changed the title Pass environment to sudo Pass environment to sudo and fix tests Dec 6, 2021
@0x2b3bfa0
Copy link
Member Author

@casperdcl, this time tests are green before the review request. 馃槄

@0x2b3bfa0 0x2b3bfa0 requested review from casperdcl and removed request for casperdcl December 6, 2021 12:14
casperdcl
casperdcl approved these changes Dec 6, 2021
View reviewed changes
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

no souls were promised nor transferred in the granting of this approval

@0x2b3bfa0 0x2b3bfa0 merged commit d20d4aa into v1 Dec 6, 2021
@0x2b3bfa0 0x2b3bfa0 deleted the sudo-e branch December 6, 2021 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperdcl casperdcl casperdcl approved these changes

Assignees

@0x2b3bfa0 0x2b3bfa0

Labels
p0-critical bugs & errors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incompatible version of node in latest GitHub ubuntu 20.04 image
2 participants
@0x2b3bfa0 @casperdcl