conda-forge / conda-forge.github.io

The conda-forge website.
https://conda-forge.org
BSD 3-Clause "New" or "Revised" License
129 stars 275 forks source link

rerender action not pushing #1064

Open xhochy opened 4 years ago

xhochy commented 4 years ago

We see several occasions where the rerender action is not pushing. The latest is in https://github.com/conda-forge/yq-feedstock/runs/689849459?check_suite_focus=true

cc @beckermr @kmuehlbauer

beckermr commented 4 years ago

This is because the PR is very old and so the actions file is not yet renamed on it.

Ugh what a mess.

I'd add a bot-rerun label.

cc @isuruf do we have any more ideas here? I suspect github will not allow any actions data to be edited by the action, the filename or otherwise.

beckermr commented 4 years ago

We could add a deploy key for each repo to use for github pushes from the actions.

beckermr commented 4 years ago

We could set up a secondary rerender service that gets triggered when the action one fails.

isuruf commented 4 years ago

We could set up a secondary rerender service that gets triggered when the action one fails.

Or revert changes to .github/workflows in the github action.

beckermr commented 4 years ago

That would also work. Do you have a preference?

beckermr commented 4 years ago

I like the deploy key the most since it actually solves the problem without relying on ppl to change things. It also makes the rerender service more robust.

I am also worried reverting changes to the actions will be finicky since we'll need to compare to the target branch of the PR.

isuruf commented 4 years ago

I am also worried reverting changes to the actions will be finicky since we'll need to compare to the target branch of the PR.

No, we just need to undo the changes done in the rerender. No need to compare with target branch.

I have a slight preference for this approach over deploy key as generating deploy keys and adding might be another source of failure in staged-recipes.

beckermr commented 4 years ago

No, we just need to undo the changes done in the rerender. No need to compare with target branch.

Really? I am confused then on what exactly github is checking. I had assumed they were checking the PR against the target. I guess you are saying they only check the incoming diff for changes to the actions.

beckermr commented 4 years ago

One more thing to consider is that if we ever need to move CI builds to github actions, we'll need to be able to actually edit them in rerendering. Adding deploy keys now might be a good insurance policy that we can do this quickly if we need to.

isuruf commented 4 years ago

I guess you are saying they only check the incoming diff for changes to the actions.

Yes.

One more thing to consider is that if we ever need to move CI builds to github actions, we'll need to be able to actually edit them in rerendering.

Why would you need to do rerendering in a CI build?

beckermr commented 4 years ago

We would not. However with changing the rerender action to not push changes to the actions, then we can't ever write a version of smithy that changes how the actions run without also pushing those changes out via the admin-service. Right now the changes we push in the admin service are repo independent, but if we were to move CI services to github actions, they would no longer be CI independent.

isuruf commented 4 years ago

Sure. Ironically though, to implement that we'd need my proposal provisionally.

beckermr commented 4 years ago

Why? I think now if we pushed a change to the rerender action via the admin migrations service to use the deploy key, it would just work.

beckermr commented 4 years ago

The rerender uses the version of the action on master.

beckermr commented 4 years ago

For example, automerge with the new action on master is working just fine, regardless of what is in the PR.

isuruf commented 4 years ago

Yes, but we get errors in rerendering. Wouldn't we get the same error because we need to change the rerender action again to use the deploy key?

isuruf commented 4 years ago

nvm, you are right.

beckermr commented 4 years ago

The error comes from the fact that we are using the github token provided by GitHub to do the push. Once we use another form of authentication, it goes away IIUIC. The admin migrations service uses a bot token to write the new action straight to every upstream branch. Thus the new action is in place with a deploy key right away. The next rerender that comes through will get trigger the repo dispatch even on master and use the updated action with the deploy key used to push.

beckermr commented 4 years ago

ahhh missed your note

beckermr commented 4 years ago

I have a slight preference for this approach over deploy key as generating deploy keys and adding might be another source of failure in staged-recipes.

This is a real concern.

beckermr commented 4 years ago

We will be adding CFEP-13 token registration to staged recipes in the next few weeks, which will tax our system even more at some level.

beckermr commented 4 years ago

cc @conda-forge/core any opinions on this?

@isuruf would you be ok if we went the deploy key route?

isuruf commented 4 years ago

Sure. Go ahead.

viniciusdc commented 3 years ago

@beckermr I think we may close this right?

beckermr commented 3 years ago

Nope. We should keep it open.

viniciusdc commented 3 years ago

Nope. We should keep it open.

The deploy key route commented didn't work ?

beckermr commented 3 years ago

Not done and would spam all of core with emails.