Skip to content

ci: Added git auto commit to workflow #27

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

Merged
merged 6 commits into from
Jan 21, 2022
Merged

ci: Added git auto commit to workflow #27

merged 6 commits into from
Jan 21, 2022

Conversation

sairamkiran9
Copy link
Contributor

Hi @DenverCoder1, In this PR I have updated the workflow script to update the docs automatically during the run after pushing the changes as per the mentioned issue 21.

@DenverCoder1 DenverCoder1 linked an issue Jan 17, 2022 that may be closed by this pull request
Copy link
Owner

@DenverCoder1 DenverCoder1 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Perhaps this should be in a separate workflow that runs only on push to main and not on pull requests?

@DenverCoder1 DenverCoder1 added the documentation Improvements or additions to documentation label Jan 17, 2022
@sairamkiran9
Copy link
Contributor Author

Thanks for the contribution!

Perhaps this should be in a separate workflow that runs only on push to main and not on pull requests?

I didn't get why we need a separate workflow for docs update due to the below concerns

  • The current workflow runs on every push to main (refer). So if the style list is not updated during the PR build, it will not get updated after merging the side branch into main.
  • In general, we commit changes in the side branch and push it to the main. And making changes directly in main is not good practice. With the existing workflow, all the updates and commits to the main will be done before merging the pull requests. So in my point of view, a separate workflow is not required.

Let me know your views and how to proceed.

@DenverCoder1
Copy link
Owner

DenverCoder1 commented Jan 19, 2022

It will not necessarily always be possible for the github action to commit to a side branch since the branch could be in a repo that the action does not have authentication to push to. The only way to guarantee it will work is if the bot were to commit to main after the PR is merged, or alternatively the bot could open a new PR to update the style list if it has changed.

An alternative approach may be to have the bot check if the styles.rst needs to be updated (if its run of the script does not match the current file contents) and have it cause the check to fail so that the user who opened the PR should be aware that they need to run the script on their branch before it can be merged. (In this way, the commit is not automated, but rather the user will be aware that they should run the script.)


I have gotten around to testing the script and there are a few things that need to be fixed for it to work.

  1. The Python script failed due to an import error.
    from table2ascii import PresetStyle, table2ascii
ModuleNotFoundError: No module named 'table2ascii'

I suggest this could be fixed by adding the top level of the repo to the path on the 2nd line of the Python script

import os
 __import__("sys").path.append(os.path.join(os.path.dirname(__file__), "..", ".."))
  1. After running the script, the git push failed.
  git push
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.8.12/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.8.12/x64/lib
fatal: You are not currently on a branch.
To push the history leading to the current (detached HEAD)
state now, use

    git push origin HEAD:<name-of-remote-branch>

I suppose the branch would need to be specified.


Let me know your thoughts on the different options of how this should proceed.

Thanks!

@DenverCoder1
Copy link
Owner

DenverCoder1 commented Jan 19, 2022

pip install table2ascii won't always work since the latest version of the code is not necessarily on pypi, so the latest changes to styles would be missing.

Instead, I think the top level directory of the repo should be added to sys.path so it can use the latest version of the codebase as suggested above.

(This will also make it easier to manually be run)

@sairamkiran9
Copy link
Contributor Author

Hi, I'm not able to fix the git push error yet. I'm new to git workflow, need some time to fix it.

For the workflow, it would be better if the bot runs and update the style.rst after every merge to main. But I don't see an option in workflow that triggers the script after merge. Let me know the approach and final thoughts to proceed.

Thanks!

@DenverCoder1
Copy link
Owner

DenverCoder1 commented Jan 19, 2022

As far as fixing the push error, maybe you can simplify by using an existing action step such as EndBug/add-and-commit. That should handle the commiting/pushing automatically.

For making it run on merge, the way to do that is with checking for push to main.

Like I suggested originally, you can make a separate workflow that just has push main and not pull requests.

on:
  workflow_dispatch:
  push:
    branches:
      - main

@sairamkiran9
Copy link
Contributor Author

As far as fixing the push error, maybe you can simplify by using an existing action step such as EndBug/add-and-commit. That should handle the commiting/pushing automatically.

For making it run on merge, the way to do that is with checking for push to main.

Like I suggested originally, you can make a separate workflow that just has push main and not pull requests.

on:
  workflow_dispatch:
  push:
    branches:
      - main

Hi @DenverCoder1, thanks for the help :)
I am able to solve the git push error using EndBug/add-and-commit. As suggested I created a new workflow that update docs on push to main. Let me know if any changes needed.
Thanks!

Copy link
Owner

@DenverCoder1 DenverCoder1 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the contribution 🎉

@DenverCoder1 DenverCoder1 changed the title added git auto commit to workflow ci: Added git auto commit to workflow Jan 21, 2022
@DenverCoder1 DenverCoder1 merged commit 9829e77 into DenverCoder1:main Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate style list docs automatically on push to main
2 participants