-
Notifications
You must be signed in to change notification settings - Fork 8
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
CI: refactor pull_request_target
to pull_request
#6
Comments
/start |
Tips:
|
@rndquu I believe it is arbitrary that you opened this issue in the repository since it impact plenty of them. Maybe would make more sense to open it at https://github.com/ubiquity/ts-template ? |
I think its related to deploys which technically has code spanning across the ts-template and every repository as an extension of the template. However this repository I think makes the most sense for deploys related tasks. |
Hi @gentlementlegen good luck! I encountered a chicken-and-egg problem for Knip reporter part here: ubiquity/keygen.ubq.fi#5 (comment) , still experimenting on that.. |
You may want to read a few links from that thread. Basically, for branches across the same repository it does work correctly with pull_request, https://github.com/gitcoindev/keygen.ubq.fi/actions/runs/8355152504 In case of |
My last version that 'almost works' is to use pull_request_workflow and a fine grained token https://github.com/korrrba/keygen.ubq.fi/actions/runs/8366591693/workflow , see tibdex/github-app-token@v2.1.0 . But perhaps I leave more investigation to you, you may discover something that works better. For the deploy workflow @rndquu was able to solve it in an elegant way, building with read only permissions and deploying with the elevated ones. |
@gitcoindev Thanks not very reassuring haha. That is correct, forks are not receiving the secrets for security purposes. The main reason why we require the secrets is also to post to the current PR the results. This could eventually be disabled, and we would still notice that the workflow failed to run. But in the case of testing, if credentials are needed, we obiously cannot expose them. I thought maybe we should either:
|
Yes I have similar conclusions. By the way in one of the articles I found a hack that allows to workaround for
When the |
Doesn't this introduce the security breach that @rndquu had been mentioning before? |
To simplify permissions what if we just use |
Wouldn't solve scenarios requiring keys or credentials |
I also thought about this approach, but for the Knip case two write permissions are needed:
If we skip comments, |
There is a quite long thread in GitHub actions repository, actions/checkout#518 Many projects struggle with that. One of the last comments mentions the similar workflow that @rndquu introduced: actions/checkout#518 (comment) (does not work for Knip-reporter currently though due to https://github.com/Codex-/knip-reporter/blob/main/src/main.ts#L24 and the fact that it complains about |
One solution I thought of as well, is using environments for our secrets to keep using |
This check is introduced because PR number is used here. The thing is that on We could update https://github.com/Codex-/knip-reporter/blob/1d32edf5aecf8728aae419758f2f4b4e35fd6945/src/main.ts to work with |
This is true, but doesn't solve cases where we need credentials or any other kind of sensitive data (I think mostly about login tests). If we use environments for our secrets we can have workflows like |
This all started with https://github.com/Codex-/knip-reporter, I would remove it, keep using plain
We don't need sensitive credentials anywhere, the only ones I can think of are cloudflare credentials but they are already used safely
I'm sure there is a way to mock github oauth without passing sensitive credentials in a build step |
It is not possible to fake the OAuth except if we also run Supabase inside the Action, and setup a whole new DB. Which would also probably require GitHub keys to set the OAuth. I would like not to have any sensitive data in the env, but we should then consider not doing any test requiring authentication, or involving any sensitive data to achieve so |
Sorry for necroposting here, but I think this has been mostly resolved as we do not use However, for The only Actions that should require environment variables are the ones requiring secrets for deployments and such, which are currently triggered after the |
This is expected and why I like to keep all of our work on GitHub so that we can use all conversations/issues as an archive.
Yes this is preferred so as to not clutter the conversation. |
Right now there are only 2 places left which uses |
@rndquu Okay these can be removed safely, I will take them of them. |
This can probably be automated with a script locally:
sed 's/pull_request_target/pull_request/g' in.yml > in.yml
git commit -m "chore: pull_request_target to pull_request
resolves https://github.com/ubiquity/cloudflare-deploy-action/issues/6"
git push |
@0x4007 Didn't you open an issue about synching the templates? This should also solve that. |
|
+ Evaluating results. Please wait... |
! Failed to run comment evaluation. SyntaxError: Unexpected end of JSON input |
I think I'll need to add funds to the old bot as well because the new bot is unreliable. It broke twice tonight already |
@gentlementlegen the deadline is at 2024-08-20T05:59:43.452Z |
+ Evaluating results. Please wait... |
|
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Task | 1 | 100 |
Issue | Comment | 10 | 0 |
Review | Comment | 1 | 0 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
@rndquu I believe it is arbitrary that you opened this issue in … | 0content: p: count: 31 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.2 | - |
@gitcoindev Thanks not very reassuring haha. That is correct, fo… | 0content: p: count: 118 score: 1 code: count: 1 score: 1 ul: count: 81 score: 0 li: count: 81 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.8 | - |
Doesn't this introduce the security breach that @rndquu had been… | 0content: p: count: 12 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.5 | - |
Wouldn't solve scenarios requiring keys or credentials | 0content: p: count: 7 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.1 | - |
One solution I thought of as well, is using environments for our… | 0content: p: count: 56 score: 1 code: count: 3 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.7 | - |
This is true, but doesn't solve cases where we need credentials … | 0content: p: count: 73 score: 1 code: count: 4 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.6 | - |
It is not possible to fake the OAuth except if we also run Supab… | 0content: p: count: 65 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.3 | - |
Sorry for necroposting here, but I think this has been mostly re… | 0content: p: count: 135 score: 1 code: count: 5 score: 1 a: count: 6 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.6 | - |
@rndquu Okay these can be removed safely, I will take them of th… | 0content: p: count: 13 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.3 | - |
@0x4007 Didn't you open an issue about synching the templates? T… | 0content: p: count: 15 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.1 | - |
Resolves https://github.com/ubiquity/cloudflare-deploy-action/is… | 0content: p: count: 3 score: 1 ul: count: 18 score: 0 li: count: 18 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.2 | - |
[ 7.3 WXDAI ]
@0x4007
Contributions Overview
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Comment | 6 | 7.3 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
I think its related to deploys which technically has code spanni… | 3.6content: p: count: 36 score: 1 wordValue: 0.1 formattingMultiplier: 1 | 0.3 | 1.08 |
To simplify permissions what if we just use `github-actions[… | 2.6content: p: count: 25 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1 | 0.6 | 1.56 |
This is expected and why I like to keep all of our work on GitHu… | 3.6content: p: count: 36 score: 1 wordValue: 0.1 formattingMultiplier: 1 | 0.2 | 0.72 |
This can probably be automated with a script locally: 1. Recur… | 4.1content: p: count: 9 score: 1 ol: count: 13 score: 0 li: count: 13 score: 1 code: count: 19 score: 1 pre: count: 16 score: 0 wordValue: 0.1 formattingMultiplier: 1 | 0.9 | 3.69 |
https://github.com/ubiquity/ts-template/issues/54 | 0.1content: p: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1 | 0.1 | 0.01 |
I think I'll need to add funds to the old bot as well because th… | 2.4content: p: count: 24 score: 1 wordValue: 0.1 formattingMultiplier: 1 | 0.1 | 0.24 |
[ 6.1475 WXDAI ]
@gitcoindev
Contributions Overview
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Comment | 6 | 6.1475 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
Hi @gentlementlegen good luck! I encountered a chicken-and-egg p… | 0.5content: p: count: 20 score: 1 wordValue: 0.1 formattingMultiplier: 0.25 | 0.2 | 0.1 |
You may want to read a few links from that thread. Basically, fo… | 1.725content: p: count: 66 score: 1 code: count: 3 score: 1 wordValue: 0.1 formattingMultiplier: 0.25 | 0.9 | 1.5525 |
My last version that 'almost works' is to use pull_request_workf… | 1.5content: p: count: 60 score: 1 wordValue: 0.1 formattingMultiplier: 0.25 | 0.7 | 1.05 |
Yes I have similar conclusions. By the way in one of the article… | 1.725content: p: count: 51 score: 1 code: count: 18 score: 1 pre: count: 10 score: 0 wordValue: 0.1 formattingMultiplier: 0.25 | 0.8 | 1.38 |
I also thought about this approach, but for the Knip case two wr… | 1.75content: p: count: 63 score: 1 pre: count: 5 score: 0 code: count: 7 score: 1 wordValue: 0.1 formattingMultiplier: 0.25 | 0.6 | 1.05 |
There is a quite long thread in GitHub actions repository, https… | 1.45content: p: count: 57 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 0.25 | 0.7 | 1.015 |
[ 38.4 WXDAI ]
@rndquu
Contributions Overview
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Specification | 1 | 21 |
Issue | Comment | 3 | 17.4 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
Right now we're using `pull_request_target` in many plac… | 21content: p: count: 9 score: 1 code: count: 4 score: 1 ul: count: 57 score: 0 li: count: 57 score: 1 wordValue: 0.1 formattingMultiplier: 3 | 1 | 21 |
[This](https://github.com/Codex-/knip-reporter/blob/1d32edf5aecf… | 10.6content: p: count: 48 score: 1 a: count: 3 score: 1 code: count: 2 score: 1 wordValue: 0.2 formattingMultiplier: 1 | 0.8 | 8.48 |
This all started with https://github.com/Codex-/knip-reporter, I… | 16content: p: count: 77 score: 1 code: count: 2 score: 1 a: count: 1 score: 1 wordValue: 0.2 formattingMultiplier: 1 | 0.4 | 6.4 |
Right now there are only 2 places left which uses `pull_requ… | 4.2content: p: count: 16 score: 1 code: count: 3 score: 1 ul: count: 2 score: 0 li: count: 2 score: 1 wordValue: 0.2 formattingMultiplier: 1 | 0.6 | 2.52 |
|
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Comment | 6 | 30.4 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
> @rndquu I believe it is arbitrary that you opened this issu... | 3.7 | 0.455 | 3.7 |
To simplify permissions what if we just use `github-actions[... | 3.7code: count: 1 score: "1" words: 3 | 0.5 | 3.7 |
> Sorry for necroposting hereThis is expected and why I l... | 3.7 | 0.6 | 3.7 |
> What should be done: | 15li: count: 4 score: "4" words: 44 code: count: 6 score: "6" words: 5 | 0.625 | 15 |
> @0x4007 Didn't you open an issue about synching the templat... | 0.8 | 0.61 | 0.8 |
> ```diff > ! Failed to run comment evaluation... | 3.5code: count: 1 score: "1" words: 0 | 0.645 | 3.5 |
[ 183.3 WXDAI ]
@gentlementlegen
Contributions Overview
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Task | 1 | 100 |
Issue | Comment | 10 | 0 |
Issue | Comment | 10 | 83.3 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
@rndquu I believe it is arbitrary that you opened this issue in ... | - | 0.43 | - |
@gitcoindev Thanks not very reassuring haha. That is correct, fo... | -li: count: 2 score: "0" words: 83 code: count: 1 score: "0" words: 3 | 0.67 | - |
Doesn't this introduce the security breach that @rndquu had been... | - | 0.505 | - |
> To simplify permissions what if we just use `github-act... | -code: count: 1 score: "0" words: 3 | 0.595 | - |
One solution I thought of as well, is using environments for our... | -code: count: 3 score: "0" words: 3 | 0.615 | - |
> [This](https://github.com/Codex-/knip-reporter/blob/1d32edf... | -a: count: 3 score: "0" words: 3 code: count: 3 score: "0" words: 6 | 0.63 | - |
It is not possible to fake the OAuth except if we also run Supab... | - | 0.595 | - |
Sorry for necroposting here, but I think this has been mostly re... | -a: count: 2 score: "0" words: 6 code: count: 5 score: "0" words: 7 | 0.615 | - |
@rndquu Okay these can be removed safely, I will take them of th... | - | 0.675 | - |
@0x4007 Didn't you open an issue about synching the templates? T... | - | 0.655 | - |
@rndquu I believe it is arbitrary that you opened this issue in ... | 3.5 | 0.43 | 3.5 |
@gitcoindev Thanks not very reassuring haha. That is correct, fo... | 23.3li: count: 2 score: "2" words: 83 code: count: 1 score: "1" words: 3 | 0.67 | 23.3 |
Doesn't this introduce the security breach that @rndquu had been... | 1.3 | 0.505 | 1.3 |
> To simplify permissions what if we just use `github-act... | 1.8code: count: 1 score: "1" words: 3 | 0.595 | 1.8 |
One solution I thought of as well, is using environments for our... | 9.9code: count: 3 score: "3" words: 3 | 0.615 | 9.9 |
> [This](https://github.com/Codex-/knip-reporter/blob/1d32edf... | 13.3a: count: 3 score: "3" words: 3 code: count: 3 score: "3" words: 6 | 0.63 | 13.3 |
It is not possible to fake the OAuth except if we also run Supab... | 6.5 | 0.595 | 6.5 |
Sorry for necroposting here, but I think this has been mostly re... | 20.8a: count: 2 score: "2" words: 6 code: count: 5 score: "5" words: 7 | 0.615 | 20.8 |
@rndquu Okay these can be removed safely, I will take them of th... | 1.3 | 0.675 | 1.3 |
@0x4007 Didn't you open an issue about synching the templates? T... | 1.6 | 0.655 | 1.6 |
[ 49.4 WXDAI ]
@gitcoindev
Contributions Overview
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Comment | 6 | 49.4 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
Hi @gentlementlegen good luck! I encountered a chicken-and-egg p... | 3 | 0.73 | 3 |
You may want to read a few links from that thread. Basically, fo... | 10.2code: count: 3 score: "3" words: 3 | 0.68 | 10.2 |
My last version that 'almost works' is to use pull_request_workf... | 7.4 | 0.525 | 7.4 |
Yes I have similar conclusions. By the way in one of the article... | 9.7code: count: 3 score: "3" words: 10 | 0.49 | 9.7 |
> To simplify permissions what if we just use `github-act... | 9.8code: count: 3 score: "3" words: 5 | 0.635 | 9.8 |
There is a quite long thread in GitHub actions repository, https... | 9.3code: count: 1 score: "1" words: 1 | 0.58 | 9.3 |
[ 83.2 WXDAI ]
@rndquu
Contributions Overview
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Specification | 1 | 32.8 |
Issue | Comment | 3 | 50.4 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
Right now we're using `pull_request_target` in many plac... | 32.8li: count: 6 score: "6" words: 74 code: count: 4 score: "4" words: 4 | 1 | 32.8 |
[This](https://github.com/Codex-/knip-reporter/blob/1d32edf5aecf... | 16.6a: count: 3 score: "3" words: 3 code: count: 2 score: "2" words: 2 | 0.645 | 16.6 |
This all started with https://github.com/Codex-/knip-reporter, I... | 19.8a: count: 1 score: "1" words: 1 code: count: 1 score: "1" words: 2 | 0.605 | 19.8 |
Right now there are only 2 places left which uses `pull_requ... | 14li: count: 2 score: "2" words: 29 code: count: 3 score: "3" words: 3 | 0.565 | 14 |
@0x4007 This was run again with the |
I think we should boost the conversation rewards. |
|
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Task | 1 | 100 |
Issue | Comment | 11 | 0 |
Review | Comment | 1 | 0 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
@rndquu I believe it is arbitrary that you opened this issue in … | 0content: p: count: 31 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.2 | - |
@gitcoindev Thanks not very reassuring haha. That is correct, fo… | 0content: p: count: 118 score: 1 code: count: 1 score: 1 ul: count: 81 score: 0 li: count: 81 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.9 | - |
Doesn't this introduce the security breach that @rndquu had been… | 0content: p: count: 12 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.4 | - |
Wouldn't solve scenarios requiring keys or credentials | 0content: p: count: 7 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.3 | - |
One solution I thought of as well, is using environments for our… | 0content: p: count: 56 score: 1 code: count: 3 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.8 | - |
This is true, but doesn't solve cases where we need credentials … | 0content: p: count: 73 score: 1 code: count: 4 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.7 | - |
It is not possible to fake the OAuth except if we also run Supab… | 0content: p: count: 65 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.5 | - |
Sorry for necroposting here, but I think this has been mostly re… | 0content: p: count: 135 score: 1 code: count: 5 score: 1 a: count: 6 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.6 | - |
@rndquu Okay these can be removed safely, I will take them of th… | 0content: p: count: 13 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.1 | - |
@0x4007 Didn't you open an issue about synching the templates? T… | 0content: p: count: 15 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.1 | - |
@0x4007 This was run again with the `max_length` fixed, … | 0content: p: count: 12 score: 1 code: count: 1 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.1 | - |
Resolves https://github.com/ubiquity/cloudflare-deploy-action/is… | 0content: p: count: 3 score: 1 ul: count: 18 score: 0 li: count: 18 score: 1 wordValue: 0 formattingMultiplier: 0 | 0.2 | - |
[ 6.49 WXDAI ]
@0x4007
Contributions Overview
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Comment | 7 | 6.49 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
I think its related to deploys which technically has code spanni… | 3.6content: p: count: 36 score: 1 wordValue: 0.1 formattingMultiplier: 1 | 0.3 | 1.08 |
To simplify permissions what if we just use `github-actions[… | 2.6content: p: count: 25 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1 | 0.4 | 1.04 |
This is expected and why I like to keep all of our work on GitHu… | 3.6content: p: count: 36 score: 1 wordValue: 0.1 formattingMultiplier: 1 | 0.1 | 0.36 |
This can probably be automated with a script locally: 1. Recur… | 4.1content: p: count: 9 score: 1 ol: count: 13 score: 0 li: count: 13 score: 1 code: count: 19 score: 1 pre: count: 16 score: 0 wordValue: 0.1 formattingMultiplier: 1 | 0.9 | 3.69 |
https://github.com/ubiquity/ts-template/issues/54 | 0.1content: p: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1 | - | - |
I think I'll need to add funds to the old bot as well because th… | 2.4content: p: count: 24 score: 1 wordValue: 0.1 formattingMultiplier: 1 | 0.1 | 0.24 |
I think we should boost the conversation rewards. | 0.8content: p: count: 8 score: 1 wordValue: 0.1 formattingMultiplier: 1 | 0.1 | 0.08 |
[ 6.2725 WXDAI ]
@gitcoindev
Contributions Overview
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Comment | 6 | 6.2725 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
Hi @gentlementlegen good luck! I encountered a chicken-and-egg p… | 0.5content: p: count: 20 score: 1 wordValue: 0.1 formattingMultiplier: 0.25 | 0.4 | 0.2 |
You may want to read a few links from that thread. Basically, fo… | 1.725content: p: count: 66 score: 1 code: count: 3 score: 1 wordValue: 0.1 formattingMultiplier: 0.25 | 0.8 | 1.38 |
My last version that 'almost works' is to use pull_request_workf… | 1.5content: p: count: 60 score: 1 wordValue: 0.1 formattingMultiplier: 0.25 | 0.6 | 0.9 |
Yes I have similar conclusions. By the way in one of the article… | 1.725content: p: count: 51 score: 1 code: count: 18 score: 1 pre: count: 10 score: 0 wordValue: 0.1 formattingMultiplier: 0.25 | 0.9 | 1.5525 |
I also thought about this approach, but for the Knip case two wr… | 1.75content: p: count: 63 score: 1 pre: count: 5 score: 0 code: count: 7 score: 1 wordValue: 0.1 formattingMultiplier: 0.25 | 0.7 | 1.225 |
There is a quite long thread in GitHub actions repository, https… | 1.45content: p: count: 57 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 0.25 | 0.7 | 1.015 |
[ 37.22 WXDAI ]
@rndquu
Contributions Overview
View | Contribution | Count | Reward |
---|---|---|---|
Issue | Specification | 1 | 21 |
Issue | Comment | 3 | 16.22 |
Conversation Incentives
Comment | Formatting | Relevance | Reward |
---|---|---|---|
Right now we're using `pull_request_target` in many plac… | 21content: p: count: 9 score: 1 code: count: 4 score: 1 ul: count: 57 score: 0 li: count: 57 score: 1 wordValue: 0.1 formattingMultiplier: 3 | 1 | 21 |
[This](https://github.com/Codex-/knip-reporter/blob/1d32edf5aecf… | 10.6content: p: count: 48 score: 1 a: count: 3 score: 1 code: count: 2 score: 1 wordValue: 0.2 formattingMultiplier: 1 | 0.8 | 8.48 |
This all started with https://github.com/Codex-/knip-reporter, I… | 16content: p: count: 77 score: 1 code: count: 2 score: 1 a: count: 1 score: 1 wordValue: 0.2 formattingMultiplier: 1 | 0.3 | 4.8 |
Right now there are only 2 places left which uses `pull_requ… | 4.2content: p: count: 16 score: 1 code: count: 3 score: 1 ul: count: 2 score: 0 li: count: 2 score: 1 wordValue: 0.2 formattingMultiplier: 1 | 0.7 | 2.94 |
Right now we're using
pull_request_target
in many places like:In the examples mentioned above workflows always check out the
development
branch + open a security hole for exfiltrating of github access token with write permissions.Check this for more info:
What should be done:
pull_request_target
topull_request
The text was updated successfully, but these errors were encountered: