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

Do not run doc testing for every push to a branch #3427

Merged
merged 3 commits into from
Oct 21, 2024
Merged

Conversation

kkraune
Copy link
Member

@kkraune kkraune commented Oct 16, 2024

Every little push to a branch is a 24-minute run, this is too excessive. The merge to master is enough for everyday use

@kkraune kkraune requested a review from esolitos October 16, 2024 06:07
@esolitos
Copy link
Contributor

🤔 I kinda disagree here, the test is there to ensure that if a guide is updated (or created) it will not be broken, the rightful place to be is in a PR.

At the very very least we should leave it for changes to the workflows. 😊

@kkraune
Copy link
Member Author

kkraune commented Oct 16, 2024

I agree in principle :-) In practice, very few PRs will change a test run. And we will see right after merge to master if it broke

Is there a middle ground here? Say in case, for a given PR, I want to trigger a run, because I am actually looking for a test run.

#3423 is an example of 90 minutes+ action run time for tiny doc changes ...

@esolitos
Copy link
Contributor

We could use the event pull_request_review, so that this is triggered only when reviews are in.

Or else we could trigger it manually using a /test comment. 🤷🏼

In either case the trigger on workflow changes should remain, because there we are directly affecting the workflows:

  pull_request:
    branches: ["master"]
    paths:
      - '.github/workflows/verify-guides.yml' # Reusable workflow
      - '.github/workflows/verify-guides-small.yml' # This workflow

@kkraune
Copy link
Member Author

kkraune commented Oct 16, 2024

added back the workflow changes.

I did not know about the /test comment - need to learn more! maybe that is a practical tool whenever we change something that needs to be tested

@esolitos
Copy link
Contributor

Well the /test needs to be implemented in the workflow, does not "just work". :D

@kkraune
Copy link
Member Author

kkraune commented Oct 21, 2024

I am merging this for now. We can always add back the PR runs for testing. lets try without and see how it goes

@kkraune kkraune merged commit 7fbe205 into master Oct 21, 2024
1 check passed
@kkraune kkraune deleted the kkraune/less-testing branch October 21, 2024 12:08
@kkraune
Copy link
Member Author

kkraune commented Oct 21, 2024

BTW, I totally agree that we should have PR testing, but for the doc workflow we could look into something quicker, lets look at alternatives

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.

2 participants