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

Fix: Scheduled test now triggers testing-stable.yml using workflow_dispatch #2196

Merged
merged 4 commits into from
Feb 15, 2024
Merged

Conversation

rohit2p
Copy link
Contributor

@rohit2p rohit2p commented Feb 10, 2024

Purpose of PR?: This pull request addresses the issue where the scheduled test was running the testing.yml workflow from the develop branch but using the container image for the stable branch. This was incorrect behavior.

Fixes #2172

Modified the testing-scheduled.yml workflow on the develop branch to directly trigger the testing-stable.yml workflow using the benc-uk/[email protected] action.

This change was made to work directly on the develop branch.

Checklist:

Copy link
Collaborator

@matrss matrss left a comment

Choose a reason for hiding this comment

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

I left a few comments. In addition to those the testing-stable.yml workflow on the stable branch would need to include the workflow_dispatch: trigger. Since that is on a different branch it must happen in an additional PR, unfortunately.

.github/workflows/testing-scheduled.yml Outdated Show resolved Hide resolved
.github/workflows/testing-scheduled.yml Show resolved Hide resolved
.github/workflows/testing-scheduled.yml Outdated Show resolved Hide resolved
@rohit2p
Copy link
Contributor Author

rohit2p commented Feb 12, 2024

@matrss i have some question, Do you prefer me to create a separate branch form the stable branch for this new PR or should i directly work on stable. Because stable branch defines the workflow for stable tests. Modifying this file directly could break the existing stable tests that's way i was confused.
the last tym when you said it need to go to develop branch i think misunderstood you and i thought you are saying me to directly work on develop branch. my apologies
also while adding the trigger is this how the code should look like??

on:
  schedule:
    - cron: '30 5 * * 1'  # Existing schedule trigger
  workflow_dispatch:      # Add the workflow_dispatch trigger
    jobs:     
      test-stable-scheduled:
        uses: 
          ./.github/workflows/testing.yml
# Rest of the code as it is

or do i need to add permission or any requirement like this?

on:
  schedule:
    - cron: '30 5 * * 1'
  workflow_dispatch:
    permissions:
      actions: write  

@rohit2p rohit2p requested a review from matrss February 12, 2024 18:36
@matrss
Copy link
Collaborator

matrss commented Feb 13, 2024

Do you prefer me to create a separate branch form the stable branch for this new PR or should i directly work on stable. Because stable branch defines the workflow for stable tests. Modifying this file directly could break the existing stable tests that's way i was confused.
the last tym when you said it need to go to develop branch i think misunderstood you and i thought you are saying me to directly work on develop branch.

What I meant with "it needs to go to develop" is that this change to the scheduled workflow ultimately needs to go into the develop branch of Open-MSS/MSS (i.e. this repository). This would happen through this PR here. What the source branch of the PR is (in this case the develop branch of your fork of this repository) is usually not important, although for my own stuff I prefer to use feature branches named after what they implement.

So what I meant regarding stable is that ultimately the testing-stable.yml workflow on the stable branch of Open-MSS/MSS needs to have the workflow_dispatch event added so that it can be triggered by the scheduled workflow running on develop.

also while adding the trigger is this how the code should look like??

on:
  schedule:
    - cron: '30 5 * * 1'  # Existing schedule trigger
  workflow_dispatch:      # Add the workflow_dispatch trigger

jobs:     
  test-stable-scheduled:
    uses: 
      ./.github/workflows/testing.yml
# Rest of the code as it is

This, but in testing-stable.yml instead of testing-scheduled.yml.

To explain what it does:
The

on:
  workflow_dispatch:

part tells GitHub Actions that the workflow that it is in can be started through a workflow_dispatch event. The benc-uk/workflow-dispatch action uses this event type to trigger an execution of that workflow. Therefore the workflow that is being started needs to "react" to this type of event.

That is also why it needs to go into stable: the testing-scheduled.yml workflow triggers the testing-stable.yml workflow on the stable branch to run. For this to successfully happen the testing-stable.yml workflow on the stable branch needs to be startable through the workflow_dispatch event.

or do i need to add permission or any requirement like this?

on:
  schedule:
    - cron: '30 5 * * 1'
  workflow_dispatch:
    permissions:
      actions: write  

No, the permission just need to be set on the job that uses the benc-uk/workflow-dispatch action (as it now already is), so that this action is allowed to start another workflow run through the workflow_dispatch event.

Does that clarify things a bit?

.github/workflows/testing-scheduled.yml Outdated Show resolved Hide resolved
@rohit2p rohit2p marked this pull request as ready for review February 13, 2024 17:49
@rohit2p rohit2p requested a review from matrss February 13, 2024 17:50
@rohit2p
Copy link
Contributor Author

rohit2p commented Feb 13, 2024

Yes, Thank you for your guidance!
Does now everything look cool?

Copy link
Collaborator

@matrss matrss left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks a lot for the PR! There is a conflict between this PR and Open-MSS:develop, which is caused by a recent commit on develop (3a0eb96) that also touched the testing-scheduled.yml workflow. If you want to fix that conflict yourself feel free to do so, otherwise this could be done when merging as well.

I've tested this change on my fork of MSS and it works so far:
https://github.com/matrss/MSS/actions/runs/7902483658
This workflow run triggered a run of testing-stable.yml that ran on stable as it should:
https://github.com/matrss/MSS/actions/runs/7902485837

@ReimarBauer what do you think about this change? It would need to be merged together with #2197.

@rohit2p
Copy link
Contributor Author

rohit2p commented Feb 14, 2024

I've resolved the conflict in the testing-scheduled.yml file by keeping the "trigger-testing-stable" job from the develop branch.

I removed the unused section related to the "test-stable-scheduled" job and cleaned up the conflict markers. Please review the changes and let me know if everything looks good.

@ReimarBauer
Copy link
Member

#2197 shows a coredump

@matrss
Copy link
Collaborator

matrss commented Feb 15, 2024

#2197 shows a coredump

See my comment in that PR.

@ReimarBauer ReimarBauer merged commit baa099e into Open-MSS:develop Feb 15, 2024
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.

The scheduled test does not test stable properly
3 participants