Skip to content

ci: implementing migrator workflow #3878

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

Open
wants to merge 119 commits into
base: main
Choose a base branch
from
Open

Conversation

germa89
Copy link
Collaborator

@germa89 germa89 commented Apr 30, 2025

Summary

As the title.

This pull request introduces a new GitHub Actions workflow, migrator.yml, designed to handle pull requests originating from forks. The workflow ensures that secrets required for CI processes can be accessed by migrating the forked PR into a branch within the base repository.

Changes

  • New Workflow: Added migrator.yml to workflows.

  • Trigger Events:

    • Automatically triggered by comments containing @pyansys-ci-bot migrate or @pyansys-ci-bot sync on pull requests.
    • Can also be triggered manually via workflow_dispatch with an issue number input.
  • Key Steps:

    • Reacts to comments to confirm or denies the action
    • Fetches details about the source branch and repository.
    • Clones the PyMAPDL repository and add the forked repo as remote.
    • Creates a new branch in the base repository, merges the forked branch, and pushes the changes.
    • Opens a new pull request in the base repository if one does not already exist.
    • Posts comments to indicate success or failure of the migration process.

This workflow simplifies the process of handling forked pull requests, improving developer experience.

Example of fork PR: #3868
Example of migrated PR: #3886

Summary by Sourcery

Implement a GitHub Actions workflow to migrate pull requests from forks, enabling secure CI processes and improving developer experience

New Features:

  • Developed automated PR migration process that creates a new branch in the base repository
  • Added comment reactions and status updates for migration workflow

CI:

  • Added migrator workflow to handle pull requests from forks
  • Implemented workflow triggered by comments or manual dispatch
  • Added member verification before migration process

Chores:

  • Created changelog entry for the migrator workflow implementation

germa89 added 23 commits April 24, 2025 11:26
…ling with improved branch checks and comments
@germa89 germa89 requested a review from a team as a code owner April 30, 2025 11:04
@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@germa89 germa89 changed the title chore: hello ci: implementing migrator workflow Apr 30, 2025
@germa89 germa89 self-assigned this Apr 30, 2025
@germa89 germa89 mentioned this pull request May 26, 2025
@germa89
Copy link
Collaborator Author

germa89 commented May 26, 2025

@RobPasMue @klmcadams @jorgepiloto @ansys/pyansys-core

I think this is now ready.

forked PR: #3955
migrated PR: #3956

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

LGTM! Saw you ended up going with raw JS + GitHub's dev package. Wasn't it easier to just use the octokit/request-action? I personally don't like JS that much but that's very opinionated 😆

path: tests_durations.json
github-token: ${{ inputs.token }}
Copy link
Member

Choose a reason for hiding this comment

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

Weird - why is the token needed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed... I saw a weird permission error when trying to download the artifacts ... so I thought about including the token there. I might have been due to recent outages though.

@germa89
Copy link
Collaborator Author

germa89 commented May 27, 2025

LGTM! Saw you ended up going with raw JS + GitHub's dev package. Wasn't it easier to just use the octokit/request-action? I personally don't like JS that much but that's very opinionated 😆

I would have been easier yes.... however I found out some troubles when trying to get the user teams. Additionally, I find the core.exportVariable quite useful, and cleaner than the bash equivalent. Also the logging, debugging, etc seems more robusts... ofc one might think "of course, that's how github is coded, it should be more complete than just using actions and bash"... so I took the opportunity to learn about it (and waste a full afternoon xD)

// Export variables for later steps
core.exportVariable('PR_NUMBER', prNumber);

if: |
(
github.event.issue.pull_request != null &&
contains(github.event.comment.body, '@pyansys-ci-bot migrate') &&
github.event.comment.user.login == 'germa89'
(contains(github.event.comment.body, '@pyansys-ci-bot migrate') || contains(github.event.comment.body, '@pyansys-ci-bot sync') )
Copy link
Member

@jorgepiloto jorgepiloto May 27, 2025

Choose a reason for hiding this comment

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

From my understanding:

  1. Outside contributor makes a push
  2. Owners with write access must approve the CI/CD run for the new push
  3. This triggers
  4. If any of previous comments, the sync gets performed

If this is the process, then it is still safe since this does not trigger unless maintainers approve the CI/CD to run.

It means that the user can not just go and make a comment. The comment is not captured until the maintainer approves the CI/CD run.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The user check is done later in the workflow. But yes, from a forked repo, each PR requires a manual approval for the CICD to run the first time. After that first run, it is no longer required in that specific PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The migrator does not need this approval since it is running in pymapdl repo, hence it is not coming from outside.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Outside contributor makes a push
  2. Owners with write access must approve the CI/CD run for the new push

Does it mean a person, external from the Ansys organization, with write access can trigger the workflow?

@jorgepiloto
Copy link
Member

Why not force pushing to the mirror pull-request? This ensures a mirror state with the original. If everything is fine in the original branch, then the mirror is fine.

Otherwise, you'll need to fix merge conflicts twice in the original and then in the mirror.

@germa89
Copy link
Collaborator Author

germa89 commented May 27, 2025

Why not force pushing to the mirror pull-request? This ensures a mirror state with the original. If everything is fine in the original branch, then the mirror is fine.

Otherwise, you'll need to fix merge conflicts twice in the original and then in the mirror.

I would expect that for some cases I might have to work also on the mirror PR inside PyMAPDL to fix stuff (code, docs, ect). So in that case, I think it is better not to force?

@germa89 germa89 requested a review from jorgepiloto May 27, 2025 14:38
Copy link
Contributor

@clatapie clatapie left a comment

Choose a reason for hiding this comment

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

I think it's a good approach!
Is it already documented in the Contribution file (from the user perspective)?
Also, we could add documentation on the process to follow for the developer perspective. It could either go in a PyMAPDL section, in the pyansys-dev-guide or somewhere else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Related with CICD, Github Actions, etc maintenance General maintenance of the repo (libraries, cicd, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants