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

Improve custom workflow documentation and add an example #319

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vchernin
Copy link
Contributor

@vchernin vchernin commented Aug 5, 2022

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

This makes it more obvious what the syntax is to add them
Multiple branches is likely a common usecase for this workflow on flathub
This requires some non-obvious configuration to work correctly, so this seems suitable here.
Originally I had thought to keep only one actual copy pastable workflow, but since some options like persist-credentials: false strictly only belong in the fork repository workflow, I thought it was more helpful to keep 2 workflows so each use case can copy paste easily.
README.md Outdated Show resolved Hide resolved
EMAIL: 41898282+github-actions[bot]@users.noreply.github.com
GITHUB_TOKEN: ${{ secrets.BOT_USER_SECRET }} # replace this with the name of the GitHub Secret where you store the PAT of the user you wish to fork the repo and create PRs
with:
args: --update --always-fork $PATH_TO_MANIFEST # e.g. com.organization.myapp.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

README.md Outdated
GIT_AUTHOR_NAME: Flatpak External Data Checker
GIT_COMMITTER_NAME: Flatpak External Data Checker
# email sets "github-actions[bot]" as commit author, see https://github.community/t/github-actions-bot-email-address/17204/6
GIT_AUTHOR_EMAIL: 41898282+github-actions[bot]@users.noreply.github.com
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gasinvein
Copy link
Collaborator

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

@wjt
Copy link
Contributor

wjt commented Aug 5, 2022

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
Copy link
Contributor Author

vchernin commented Aug 5, 2022

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.

…uires some non-obvious configuration to work correctly, so this seems suitable here. Originally I had thought to keep only one actual copy pastable workflow, but since some options like persist-credentials: false strictly only belong in the fork repository workflow, I thought it was more helpful to keep 2 workflows so each use case can copy paste easily.
@vchernin vchernin force-pushed the improve-workflow-forks-documentation branch from 11cb17e to 2a889f4 Compare August 5, 2022 14:57
@gasinvein
Copy link
Collaborator

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
Copy link
Contributor Author

vchernin commented Aug 5, 2022

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
Copy link
Collaborator

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
Copy link
Contributor Author

vchernin commented Aug 5, 2022

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
Copy link
Collaborator

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
Copy link
Contributor Author

vchernin commented Aug 5, 2022

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants