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

Bugfix/directory workflow #596

Merged
merged 5 commits into from
Dec 13, 2023

Conversation

CSRessel
Copy link
Contributor

Pull Request Template

Description

This PR attempts to address the problem of the directory workflow not writing back the repo.

The full discussion is here: #591

Before merging, @siriak should try:

  1. adding a personal access token to the repo secrets as BOT_TOKEN (classic is easiest, but if you do fine-grained make sure to give it repo permission)
  2. make sure that for the master branch protection, his own account is included in the Allow specific actors to bypass pull request requirements list

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I ran bellow commands using the latest version of rust nightly.
  • I ran cargo clippy --all -- -D warnings just before my last commit and fixed any issue that was found.
  • I ran cargo fmt just before my last commit.
  • I ran cargo test just before my last commit and all tests passed.
  • I added my algorithm to the corresponding mod.rs file within its own folder, and in any parent folder(s).
  • I added my algorithm to DIRECTORY.md with the correct link.
  • I checked COUNTRIBUTING.md and my code follows its guidelines.

Please make sure that if there is a test that takes too long to run ( > 300ms), you #[ignore] that or
try to optimize your code or make the test easier to run. We have this rule because we have hundreds of
tests to run; If each one of them took 300ms, we would have to wait for a long time.

@siriak
Copy link
Member

siriak commented Nov 1, 2023

Could you clarify a few thing for me?

  1. When adding a fine-grained access token, I only see my personal repos. I don't see TheAlgorithms repos. Do you know how to create a token for them?
    image
  2. Why does it need access to master? I'd expect it to commit to the feature branch so that changes to DIRECTORY.md can be reviewed too. Can we do it like that?

@CSRessel
Copy link
Contributor Author

CSRessel commented Nov 3, 2023

  1. I need to read more about how to give a GitHub Action write access to a repo then (if we go forward with this option). I guess it would have needed to be the account owner of TheAlgorithms account who can create a PAT with access. I can look into other options.
  2. I think there's a barrier to having the actions commit to a feature branch. The feature branch lives in the author's fork of the repo, so to push to the feature branch you need permission in that fork. There's no global configuration we can setup in this repo to allow that; the next best thing I'm aware of is encouraging contributors to enable the GitHub Actions in their fork (and then the DIRECTORY generation will happen automatically in their branch, which I confirmed in my fork).

My thinking is that to avoid the challenge of updating the DIRECTORY file across the forked repos, we could make it updated on merge to master. But I like your point, it would be great if that was part of the diff itself in the PR! I'll look into it further

@siriak
Copy link
Member

siriak commented Nov 3, 2023

  1. Getting PAT from account owner of TheAlgorithms is a profoundly complicated task to say the least, so I'd like to circumvent that if possible
  2. That's insightful. encouraging contributors to enable the GitHub Actions in their fork is not an option in my opinion, we just can't encourage a significant number of people do that.
  3. I'm fine with any approach you choose (either update in master or in PR) as long as it works and is not terribly complicated :)

Thanks for your effort, I really appreciate that!

@siriak
Copy link
Member

siriak commented Nov 3, 2023

P.S. see this comment. It seems that there may be an issue with the currently proposed approach.

Copy link

github-actions bot commented Dec 4, 2023

This pull request has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Dec 4, 2023
@CSRessel
Copy link
Contributor Author

CSRessel commented Dec 4, 2023

I still haven't found a solution to update the DIRECTORY.md file in the pull request branch, since that lives in the contributor's own forked repo (and they would need to enable GitHub Actions there to have the workflows run on their branch).

I think a better route forward that avoids the PAT problems above, would be to configure an SSH deploy key for the repo, and then provide the private key as a secret for the workflows to deploy back to the GitHub master branch itself. Although this doesn't have the benefits of previewing the directory changes in the PR diff, at least every time a PR is merged the workflows can automatically commit the directory update

Steps I would need help with from a repo admin:

  1. Create the deploy key and add to repo with write access
  2. Add the deploy private key as a secret for the repo
  3. Share the secret name here, so it can be used to update the directory_workflow.yml

@siriak does that make sense? If so would you be available to try that out?

@github-actions github-actions bot removed the stale label Dec 5, 2023
@siriak
Copy link
Member

siriak commented Dec 5, 2023

I will try doing this next weekend, I'm a little busy till then. Thank you!

@siriak
Copy link
Member

siriak commented Dec 10, 2023

Basically what these deploy keys do is they allow pushing to master. I have just allowed pushing to it (in branch protection settings). This allows a little bit more than I need, but I feel like it's way simpler than maintaining these deploy keys in the future should anything change. Could you resolve the conflicts, and then I'll merge to see how it works?

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1fd579c) 94.51% compared to head (3203f7f) 94.51%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #596   +/-   ##
=======================================
  Coverage   94.51%   94.51%           
=======================================
  Files         277      277           
  Lines       22258    22258           
=======================================
  Hits        21037    21037           
  Misses       1221     1221           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CSRessel
Copy link
Contributor Author

CSRessel commented Dec 13, 2023

Just updated merge conflicts. Wanted to ask how you added the ssh key? Is it a secret for the repo (in which case I just need to add a couple lines to use something like the web-factory/ssh-agent workflow to setup the new ssh key), or is it something just inherited by all workflows and no explicit step there is needed?

I had anticipated needing some lines like:

      - uses: webfactory/ssh-agent@v0.5.0
        with:
          ssh-private-key: ${{ secrets.DIRECTORY_DEPLOY_KEY }}

In order to explicitly use the elevated privileges with the deploy key.

@CSRessel CSRessel marked this pull request as ready for review December 13, 2023 06:49
@siriak
Copy link
Member

siriak commented Dec 13, 2023

I didn't add an ssh key. The workflow just uses some built-in authentication (I'm not terribly familiar with that)

@siriak
Copy link
Member

siriak commented Dec 13, 2023

Let's see if DIRECTORY is updated after I merge this PR

@siriak siriak merged commit 3f448d2 into TheAlgorithms:master Dec 13, 2023
4 checks passed
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.

None yet

3 participants