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

CML initialization is broken on GH in container mode #970

Closed
shcheklein opened this issue Apr 18, 2022 · 16 comments · Fixed by #974
Closed

CML initialization is broken on GH in container mode #970

shcheklein opened this issue Apr 18, 2022 · 16 comments · Fixed by #974
Assignees
Labels
p0-critical Max priority (ASAP)

Comments

@shcheklein
Copy link
Member

Basic CML workflow like this is broken.

I'm getting:

{"level":"error","message":"Command failed: git config --get remote.origin.url","output":[null,{"data":[],"type":"Buffer"},{"data":[],"type":"Buffer"}],"pid":124,"signal":null,"stack":"Error: Command failed: git config --get remote.origin.url\n    at checkExecSyncError (node:child_process:828:11)\n    at execSync (node:child_process:902:15)\n    at gitRemoteUrl (/usr/lib/node_modules/@dvcorg/cml/src/cml.js:29:15)\n    at new CML (/usr/lib/node_modules/@dvcorg/cml/src/cml.js:76:44)\n    at Object.exports.handler (/usr/lib/node_modules/@dvcorg/cml/bin/cml/ci.js:10:15)\n    at /usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:9054\n    at j (/usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:4931)\n    at M.applyMiddlewareAndGetResult (/usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:9023)\n    at M.runCommand (/usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:7206)\n    at Xt.[runYargsParserAndExecuteCommands] (/usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:57164)\n    at Xt.parse (/usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:39275)\n    at Object.<anonymous> (/usr/lib/node_modules/@dvcorg/cml/bin/cml.js:90:4)\n    at Module._compile (node:internal/modules/cjs/loader:1103:14)\n    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1157:10)\n    at Module.load (node:internal/modules/cjs/loader:981:32)\n    at Function.Module._load (node:internal/modules/cjs/loader:822:12)","status":1,"stderr":{"data":[],"type":"Buffer"},"stdout":{"data":[],"type":"Buffer"}}

Command git config --get remote.origin.url that is triggered in CML ctr fails.

Most likely this is due to some breaking changes in action checkout (v2 and v3 both affected). They have recently had to upgrade those due to recent update of Git on GH https://github.blog/2022-04-12-git-security-vulnerability-announced/

(more info for the reference is here: https://iterativeai.slack.com/archives/C01900GSB4J/p1650245948446699 (internal link))

@shcheklein shcheklein added the p0-critical Max priority (ASAP) label Apr 18, 2022
@dacbd
Copy link
Contributor

dacbd commented Apr 18, 2022

minimal example:

name: test cml ci
on:
  push:
  workflow_dispatch:

jobs:
  test-v2:
    runs-on: ubuntu-latest
    container: ghcr.io/iterative/cml:latest
    steps:
      - uses: actions/checkout@v2
      - run: cml ci
  test-v3:
    runs-on: ubuntu-latest
    container: ghcr.io/iterative/cml:latest
    steps:
      - uses: actions/checkout@v3
      - run: cml ci
  test-setup-v2:
    runs-on: ubuntu-latest
    steps:
      - uses: iterative/setup-cml@v1
      - uses: actions/checkout@v2
      - run: cml ci
  test-setup-v3:
    runs-on: ubuntu-latest
    steps:
      - uses: iterative/setup-cml@v1
      - uses: actions/checkout@v3
      - run: cml ci

https://github.com/dacbd/err/actions/runs/2182120620

test-v2 / test-v3 fail with an error:

{
  "level": "error",
  "message": "Command failed: git config --get remote.origin.url",
  "output": [
    null,
    {
      "data": [],
      "type": "Buffer"
    },
    {
      "data": [],
      "type": "Buffer"
    }
  ],
  "pid": 126,
  "signal": null,
  "stack": "Error: Command failed: git config --get remote.origin.url\n    at checkExecSyncError (node:child_process:828:11)\n    at execSync (node:child_process:902:15)\n    at gitRemoteUrl (/usr/lib/node_modules/@dvcorg/cml/src/cml.js:29:15)\n    at new CML (/usr/lib/node_modules/@dvcorg/cml/src/cml.js:76:44)\n    at Object.exports.handler (/usr/lib/node_modules/@dvcorg/cml/bin/cml/ci.js:10:15)\n    at /usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:9054\n    at j (/usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:4931)\n    at M.applyMiddlewareAndGetResult (/usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:9023)\n    at M.runCommand (/usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:7206)\n    at Xt.[runYargsParserAndExecuteCommands] (/usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:57164)\n    at Xt.parse (/usr/lib/node_modules/@dvcorg/cml/node_modules/yargs/build/index.cjs:1:39275)\n    at Object.<anonymous> (/usr/lib/node_modules/@dvcorg/cml/bin/cml.js:90:4)\n    at Module._compile (node:internal/modules/cjs/loader:1103:14)\n    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1157:10)\n    at Module.load (node:internal/modules/cjs/loader:981:32)\n    at Function.Module._load (node:internal/modules/cjs/loader:822:12)",
  "status": 1,
  "stderr": {
    "data": [],
    "type": "Buffer"
  },
  "stdout": {
    "data": [],
    "type": "Buffer"
  }
}

when I expect them to complain with same token not found as the test-setup-v2/v3

@casperdcl
Copy link
Contributor

Also (/CC @iterative/cml) think we recommend actions/checkout before setup-cml, right?

@DavidGOrtega
Copy link
Contributor

Also (/CC @iterative/cml) think we recommend actions/checkout before setup-cml, right?

Yes, actions/checkout should be prior to anything else. It's what setup the repo within the runner

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Apr 18, 2022

we recommend actions/checkout before setup-cml, right?

Order doesn't matter, unless iterative/setup-cml does something that it's not supposed (?) to do, like running cml ci on its own.

@0x2b3bfa0
Copy link
Member

Infimum example

on: workflow_dispatch
jobs:
  succeeds:
    runs-on: ubuntu-latest
    steps:
      - uses: iterative/setup-cml@v1
      - uses: actions/checkout@v3
      - run: cml ci
        env:
          GITHUB_TOKEN: ${{ github.token }}
  fails:
    runs-on: ubuntu-latest
    container: ghcr.io/iterative/cml
    steps:
      - uses: actions/checkout@v3
      - run: cml ci
        env:
          GITHUB_TOKEN: ${{ github.token }}

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Apr 18, 2022

Fix

Running git config --global --add safe.directory "$PWD" as part of cml ci is enough to fix this issue. See actions/checkout#766 for more information.

on: push
jobs:
  fix:
    runs-on: ubuntu-latest
    container: ghcr.io/iterative/cml
    steps:
      - uses: actions/checkout@v3
      - run: git config --global --add safe.directory "$PWD"
      - run: cml ci
        env:
          GITHUB_TOKEN: ${{ github.token }}

@0x2b3bfa0 0x2b3bfa0 self-assigned this Apr 18, 2022
@0x2b3bfa0
Copy link
Member

@iterative/cml & company, what option do you prefer?

  1. Add CWD to Git's safe.directory with cml ci #974 and recommend running cml ci as part of every workflow
  2. Add CWD to Git's safe.directory with cml ci #974 and run that code automatically for all the cml commands (i.e. implicit cml ci)
  3. Pin to git<2.35.2 on all our container images 🙃

@shcheklein
Copy link
Member Author

@0x2b3bfa0 are there other way to get the same result as git config --get remote.origin.url in JS code? that's the only thing that is failing, right? and it looks the only thing that we are using that for is to determine GH/GL/BB? maybe there are also other ways to do thi?

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Apr 19, 2022

@0x2b3bfa0 are there other way to get the same result as git config --get remote.origin.url in JS code?

Yes, see the parse-git-config package for example, but it's not enough to replace all our other interactions with Git.

that's the only thing that is failing, right?

It's rather the first thing that is failing. Any git command we run will produce the same error:

fatal: unsafe repository ('/path/to/repository' is owned by someone else)
To add an exception for this directory, call:

        git config --global --add safe.directory /path/to/repository

it looks the only thing that we are using that for is to determine GH/GL/BB?

I'd wish! 😅 We use many other git commands for various purposes.

@shcheklein
Copy link
Member Author

shcheklein commented Apr 19, 2022

It's rather the first thing that is failing

Got it. How does CI works in general now if any git is failing? Does it mean that it'll be fixed by GH soon? Or is it specific to the way action checkout does things?

Pin to git<2.35.2 on all our container images

Seems unreliable - we'll have to deal with this in the non-container mode now or soon?


#974

One more option - can we make it part of CML init?

@0x2b3bfa0 0x2b3bfa0 assigned dacbd and unassigned dacbd Apr 19, 2022
@0x2b3bfa0

This comment was marked as outdated.

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Apr 19, 2022

How does CI works in general now if any git is failing? [...] Seems unreliable - we'll have to deal with this in non-container more now or soon?

It works fine everywhere, except for containers, where there is a mismatch between the owner of the cloned repository and the user running the commands: actions/checkout#47 (comment)

Does it mean that it'll be fixed by GH soon? Or is it specific to the way action checkout does things

It will probably be fixed by GitHub, and it's specific to the actions/checkout action. Not sure about the soon part, though: actions/checkout#762 (comment) mentions it quite casually.

One more option - can we make it part of CML init?

If you mean setting safe.directory as part of every CML command, that would be option 2 on #970 (comment) more or less. 🤔

@shcheklein
Copy link
Member Author

shcheklein commented Apr 19, 2022

Right, thanks for clarifying this one more time, Helio.

If you mean setting safe.directory as part of every CML command, that would be option 2 on #970 (comment) more or less. 🤔

Yes, part of every command. Not exactly option 2 if I understand it right, since cml ci might be doing a bit more than that.

@0x2b3bfa0
Copy link
Member

Yes, part of every command. Not exactly option 2 if I understand it right, since cml ci might be doing a bit more than that.

Absolutely! 💯

@0x2b3bfa0
Copy link
Member

Highlights from Git 2.36

Since publishing that blog post, the safe.directory option now interprets the value * to consider all Git repositories as safe, regardless of their owner. You can set this in your --global config to opt-out of the new behavior in situations where it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p0-critical Max priority (ASAP)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants