Improve custom workflow documentation and add an example

[vchernin]

This adds an example in case wanting f-e-d-c to fork a repository to make PRs.

[wjt]

You don't strictly need --always-fork since the script will check whether it has push access to the main repo, and use a fork if not.

But I can see it is good for documentation value.

[vchernin]

Yeah, I did for documentation value especially since the other example uses --never-fork

[wjt]

These seem suspicious to me. You're providing a GITHUB_TOKEN corresponding to some real user. You should be using (a placeholder for) that user's email address.

[vchernin]

Agreed, I also changed the suggested name to match the email address given by the user for consistency.

[gasinvein]

Hmm, in what case one would want to push updates through a fork of a repo?

[wjt]

The original use case was that the Endless instance of this bot did not have write access to the Flathub repos it monitored. But that use case is long gone!

[vchernin]

Hmm, in what case one would want to push updates through a fork of a repo?

As mentioned in the suggested README:

This may be preferred if you do not want the tool to have direct push access to the canonical repository.

I mostly wanted this since some repos may not want to give bots push access to their real repo. If the bot only has the ability to push to a fork and make PRs (like a normal user on GitHub would), it avoids bad things being pushed to the repo in case the bot was compromised or buggy.

[gasinvein]

Hmm, in what case one would want to push updates through a fork of a repo?

As mentioned in the suggested README:

This may be preferred if you do not want the tool to have direct push access to the canonical repository.

Well, I believe this was intended for local or otherwise external f-e-d-c runs. If it's running from a GH workflow, I fail to understand why create a fork. Besides, a fork forces you to use a PAT, which are worse than automatic GITHUB_TOKENs from the security standpoint, since automatic tokens, as far as I'm aware, are one-time use, thus leaking them is less severe.

I mostly wanted this since some repos may not want to give bots push access to their real repo. If the bot only has the ability to push to a fork and make PRs (like a normal user on GitHub would), it avoids bad things being pushed to the repo in case the bot was compromised or buggy.

What's the bot in this case? If you mean the flathubbot, it has access to all flathub app repos anyway. And if it's a GH workflow in a non-flathub repo, then it's controlled by the repo owner anyway.

[vchernin]

Besides, a fork forces you to use a PAT, which are worse than automatic GITHUB_TOKENs from the security standpoint, since automatic tokens, as far as I'm aware, are one-time use, thus leaking them is less severe.

Yes the automatic GITHUB_TOKENs are one time use and certainly more ideal over a permanent one. But there is still a use case for using a PAT for a seperate user…

What's the bot in this case? If you mean the flathubbot, it has access to all flathub app repos anyway. And if it's a GH workflow in a non-flathub repo, then it's controlled by the repo owner anyway.

The use case I had in mind was to create a dummy bot user, for a non Flathub repo.

Yes the workflow is controlled by the repo owner, but that doesn’t prevent f-e-d-c or any other tool in the workflows from one day updating and being buggy, or other otherwise misusing their permission. By forcing the tool to push to a fork, it cannot change anything in the real repo without a reviewer accepting a PR from the bot user.

Giving permanent access to the PAT of a whole user is not ideal, but if the PAT is leaked the consequences are in my opinion not as severe compared to giving bots push access, as it is only a dummy bot user. It never had write access to anything but its own forked repo. Of course, that is only if users are following the recommendation to not use the dummy user for anything important.

Perhaps something like a GitHub app that can be added to the repo would serve this use case better than a PAT, but I’m not really familiar with how they work.

[gasinvein]

Well, I guess someone may want such setup for some reason, but it's certainly something way too unusual to be worth adding to the readme, especially with a full-blown example workflow manifest. It will only add confusion.

[vchernin]

It will only add confusion.

Yeah this is inevitable with adding more examples. What do you think of splitting these workflows, perhaps putting this new one in a separate file, and then adding something like:

If you want to see some non-standard ways of configuring the workflow go to WORKFLOW_EXAMPLES.md.

below the original example in the main readme?

but it's certainly something way too unusual to be worth adding to the readme, especially with a full-blown example workflow manifest.

I still think it is somewhat useful to document given --always-fork exists and some of the configuration (namely persist-credentials: false) isn't easy to figure out if you don’t know what to look for.

[gasinvein]

Honestly, I think it's not worth documenting with example at all. People doing something this uncommon probably either shouldn't need any examples, or shouldn't be doing it. So, I believe mentioning the --always-fork option and describing what it does should be enough.

[vchernin]

So, I believe mentioning the --always-fork option and describing what it does should be enough

By this you mean adding an additional line about --always-fork in the custom workflow section of the readme?

[gasinvein]

So, I believe mentioning the --always-fork option and describing what it does should be enough

By this you mean adding an additional line about --always-fork in the custom workflow section of the readme?

Yes, I think this should be enough for this option.