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

📝 Update docstring generator to include target branch for push #477

Merged
merged 9 commits into from
Jul 19, 2024

Conversation

Drewniok
Copy link
Collaborator

Description

The docstring generator workflow successfully runs when merging branches originating from upstream-main. However, it fails to push docstrings during workflow execution for branches originating from fork-main due to detached branches.

This PR resolves the issue by specifying the target branch for pushing docstrings.

Thanks @simon1hofmann for helping to figure this out!

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have added a changelog entry.
  • I have created/adjusted the Python bindings for any new or updated functionality.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@Drewniok Drewniok added the bug Something isn't working label Jul 18, 2024
@Drewniok Drewniok self-assigned this Jul 18, 2024
@Drewniok Drewniok changed the title 📝 Add branch name to push changes 📝 Add branch name to docstring generator to push changes Jul 18, 2024
@Drewniok Drewniok changed the title 📝 Add branch name to docstring generator to push changes 📝 Update docstring generator to include target branch for push Jul 18, 2024
@Drewniok Drewniok marked this pull request as draft July 18, 2024 14:55
@simon1hofmann
Copy link
Collaborator

Then the changes of #475 can probably be reverted right?
on: push should be enough.

@Drewniok Drewniok marked this pull request as ready for review July 18, 2024 15:04
@Drewniok Drewniok requested a review from marcelwa July 18, 2024 15:04
@simon1hofmann
Copy link
Collaborator

I think it's not possible to push to the branch like that, also add-and-commit should take care of that automatically.

@Drewniok
Copy link
Collaborator Author

ah great! Thank you!

@marcelwa
Copy link
Collaborator

@Drewniok @simon1hofmann does this work as intended now?

@Drewniok
Copy link
Collaborator Author

@marcelwa Not yet

@Drewniok Drewniok removed the request for review from marcelwa July 19, 2024 07:48
@marcelwa
Copy link
Collaborator

Alright, keep me posted

@Drewniok Drewniok requested a review from marcelwa July 19, 2024 08:07
@Drewniok
Copy link
Collaborator Author

@marcelwa It works now! Thanks @simon1hofmann for your help.

@@ -20,6 +16,7 @@ jobs:
uses: actions/checkout@v4
with:
submodules: recursive
fetch-depth: 0 # Fetch all history for all branches and tags
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this important?

Copy link
Collaborator

@simon1hofmann simon1hofmann Jul 19, 2024

Choose a reason for hiding this comment

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

Previously, the docstring generator only worked for branches forked directly from fiction and not, e.g., Drewniok/fiction, which lead to this error message:

Error: Error: 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>

By default, fetch-depth is set to 1, so something similar like this happens:
git fetch --depth 1 origin $GITHUB_REF

I guess this misses the forked branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood. Many thanks!

@marcelwa marcelwa merged commit 2379031 into cda-tum:main Jul 19, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants