search

iterative / setup-cml

GitHub Action for CML setup
25 stars 12 forks source link

Pass environment to `sudo` and fix tests #55

Closed 0x2b3bfa0 closed 2 years ago

0x2b3bfa0 commented 2 years ago

May close #53; fixes bugs in the testing workflow. 🐛🔨

0x2b3bfa0 commented 2 years ago

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 commented 2 years ago

Tested and working as expected.

0x2b3bfa0 commented 2 years ago

@casperdcl, it looks like I need you to approve again the pull request.

casperdcl commented 2 years ago

~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 commented 2 years ago

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 commented 2 years ago

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

0x2b3bfa0 commented 2 years ago

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