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

support comment --update with default GITHUB_TOKEN #1105

Closed
casperdcl opened this issue Jul 26, 2022 · 14 comments
Closed

support comment --update with default GITHUB_TOKEN #1105

casperdcl opened this issue Jul 26, 2022 · 14 comments
Assignees
Labels
ci-github cml-comment Subcommand external-request You asked, we did p1-important High priority

Comments

@casperdcl
Copy link
Contributor

casperdcl commented Jul 26, 2022

CML comment --update requires a user-defined PAT. Can we change the implementation to work with the default token too?

Would help a lot with user-friendliness of #1017.

@rogermparent
Copy link

From iterative/cml.dev#271 (comment)

The one thing I could see in the code is that create-or-update-comment gets its octokit through @actions/github's core.getOctokit while CML's GitHub driver gets it through @octokit/rest's Octokit constructor, seemingly in order to add the throttling plugin.

@DavidGOrtega
Copy link
Contributor

I think that this limitation is a GH issue. peter-evans/create-or-update-comment only works with issue comment that are inherently different than commits comments.
The link of both differs and indeed peter-evans/create-or-update-comment have another error when updating commits comments.

In example:

https://github.com/DavidGOrtega/fashion_mnist/commit/1ea79b838635928375cf459b978b6e1116cf13cd#commitcomment-79652370
Run peter-evans/create-or-update-comment@v2
  with:
    comment-id: 7965[2](https://github.com/DavidGOrtega/fashion_mnist/runs/7577465708?check_suite_focus=true#step:3:2)[3](https://github.com/DavidGOrtega/fashion_mnist/runs/7577465708?check_suite_focus=true#step:3:3)70
    body: echo Hi from Action!
  
    token: ***
  env:
    AWS_ACCESS_KEY_ID: ***
    AWS_SECRET_ACCESS_KEY: ***
Error: Not Found

While

https://github.com/DavidGOrtega/fashion_mnist/issues/37#issuecomment-1199153152
Run peter-evans/create-or-update-comment@v2
  with:
    comment-id: 119915315[2](https://github.com/DavidGOrtega/fashion_mnist/runs/7577674253?check_suite_focus=true#step:3:2)
    body: echo Hi from Action!
  
    token: ***
  env:
    AWS_ACCESS_KEY_ID: ***
    AWS_SECRET_ACCESS_KEY: ***
Updated comment id '119915[3](https://github.com/DavidGOrtega/fashion_mnist/runs/7577674253?check_suite_focus=true#step:3:3)1[5](https://github.com/DavidGOrtega/fashion_mnist/runs/7577674253?check_suite_focus=true#step:3:5)2'.

Want I want to say is that both comments are different in terms of API calls and it does not means anything that the actoin can update issue comments, the question is if thats possible.
Im testing the API via CURL

@DavidGOrtega
Copy link
Contributor

As I expected

curl -X PATCH \
        -H "Accept: application/vnd.github+json" \
        -H "Authorization: token ${GITHUB_TOKEN}" \
        https://api.github.com/repos/DavidGOrtega/fashion_mnist/comments/79652370 \
        -d '{"body":"HI CURL"}'
Run curl -X PATCH \
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
{
  "message": "Validation Failed",
  "errors": [
    {
100   3[13](https://github.com/DavidGOrtega/fashion_mnist/runs/7578675874?check_suite_focus=true#step:4:14)  100   295  100    [18](https://github.com/DavidGOrtega/fashion_mnist/runs/7578675874?check_suite_focus=true#step:4:19)    993     60 --:--:-- --:--:-- --:--:--  1053
      "resource": "CommitComment",
      "code": "custom",
      "field": "commit_id",
      "message": "commit_id has been locked"
    }
  ],
  "documentation_url": "https://docs.github.com/rest/reference/repos#update-a-commit-comment"
}

@dacbd
Copy link
Contributor

dacbd commented Jul 29, 2022

Again, espescially for the github api, comment on an issue differs from a comment on a commit.

cml/src/drivers/github.js

Lines 102 to 107 in 8aeac20

await repos.createCommitComment({
...ownerRepo({ uri: this.repo }),
commit_sha: commitSha,
body
})
).data.html_url;

https://github.com/peter-evans/create-or-update-comment/blob/b95e16d2859ad843a14218d1028da5b2c4cbc4b4/index.js#L137-L142

@DavidGOrtega
Copy link
Contributor

@dacbd thats what I say. GH api behaves different with an issue or a commit comment.
We are not doing anything wrong unless what we want is to be able to create ISSUE comments but CML was not originally thought for that.

@0x2b3bfa0
Copy link
Member

@0x2b3bfa0
Copy link
Member

Can users solve this with custom permissions perhaps?

@DavidGOrtega
Copy link
Contributor

Can users solve this with custom permissions perhaps?

Whats the difference of creating a PAT then? CML needs a PAT for the runner

@0x2b3bfa0
Copy link
Member

It could be better for use cases where runners aren't needed. 🤷🏼‍♂️

@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented Jul 29, 2022

But... there is an extra effort right ? I mean, you can not do this with a common GITHUB_TOKEN you need to sort out something else and probably you need the same permissions to modify it than to create a PAT... I do not fully understand the real use of GITHUB_TOKEN when you can create a PAT

@DavidGOrtega
Copy link
Contributor

In any case there is nothing we can do to do on our side to fix send-comment.
Feel free to open it if makes sense

@casperdcl
Copy link
Contributor Author

casperdcl commented Aug 5, 2022

To clarify, @DavidGOrtega do you mean default (non-PAT) GITHUB_TOKEN is sufficient to:

location creating comment updating comment
commit
PR
issue ✔️ ✔️

Are you sure?

From https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token it seems to imply Default access (permissive) mean:

location creating comment updating comment
commit ✔️ ✔️
PR ✔️ ✔️
issue ✔️ ✔️

while Default access (restricted) mean:

location creating comment updating comment
commit
PR
issue

@rogermparent
Copy link

Again, espescially for the github api, comment on an issue differs from a comment on a commit.

There's also an action for creating commit comments that also doesn't seem to require a PAT.

@dacbd
Copy link
Contributor

dacbd commented Aug 5, 2022

Again, espescially for the github api, comment on an issue differs from a comment on a commit.

There's also an action for creating commit comments that also doesn't seem to require a PAT.

Indeed @rogermparent it looks like you are correct and I would say that it appears cml send-comment works fine with the default github token

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-github cml-comment Subcommand external-request You asked, we did p1-important High priority
Projects
None yet
Development

No branches or pull requests

5 participants