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.

Sign up for GitHub

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

Jump to bottom

🌱 [ci]: Skipping jobs which are not required when the changes are in only docs #2608

Merged
merged 1 commit into from
May 14, 2022
Merged

🌱 [ci]: Skipping jobs which are not required when the changes are in only docs #2608

merged 1 commit into from
May 14, 2022

Conversation

NikhilSharmaWe
Copy link
Member

@NikhilSharmaWe NikhilSharmaWe commented Apr 10, 2022 •
edited
Loading

Description

There is no need of running all jobs when the changes are only in docs. This PR skips the github actions which are not required in those cases.

Examples :
PR making changes in go files runs all tests: NikhilSharmaWe#31
PR making changes in md files skip jobs which are not required: NikhilSharmaWe#30

Motivation

Fixes Part of: #2591

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 10, 2022
@NikhilSharmaWe NikhilSharmaWe changed the title ✨ Changes to not run tests when the changes are only in docs, comments, etc. ✨ Don't need to run tests when the changes are only in docs, comments, etc. Apr 10, 2022
@NikhilSharmaWe
Copy link
Member Author

NikhilSharmaWe commented Apr 11, 2022 •
edited
Loading

@joelanford @camilamacedo86 Could you please review the changes made.
Is the approach used reasonable?

@NikhilSharmaWe NikhilSharmaWe changed the title ✨ Don't need to run tests when the changes are only in docs, comments, etc. ✨ (ci): Don't need to run tests when the changes are only in docs, comments, etc. Apr 11, 2022
@NikhilSharmaWe
Copy link
Member Author

/assign @joelanford

@camilamacedo86
Copy link
Member

@NikhilSharmaWe,

That is great. Could you please add the description of the 2 PR to test these changes in your fork?
One pr needs to do all tests without skipping when has a change in a go file
Another needs to skip when the tests when the change is only in the docs

@NikhilSharmaWe
Copy link
Member Author

NikhilSharmaWe commented Apr 12, 2022 •
edited
Loading

@camilamacedo86 I think there is some issue here because tests are running in both cases (not e2e).
PR changing md file NikhilSharmaWe#8
PR changing go file NikhilSharmaWe#7

Could you please explain what is the issue here with the changes. Or there is some other issue with repo settings.

camilamacedo86
camilamacedo86 reviewed Apr 13, 2022
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All sorted out.

@NikhilSharmaWe NikhilSharmaWe changed the title ✨ (ci): Don't need to run tests when the changes are only in docs, comments, etc. 🌱 (ci): Don't need to run tests when the changes are only in docs, comments, etc. Apr 13, 2022
@NikhilSharmaWe NikhilSharmaWe changed the title 🌱 (ci): Don't need to run tests when the changes are only in docs, comments, etc. 🌱 Skipping jobs which are not required when the changes are in only docs Apr 13, 2022
@NikhilSharmaWe NikhilSharmaWe changed the title 🌱 Skipping jobs which are not required when the changes are in only docs 🌱 [ci]: Skipping jobs which are not required when the changes are in only docs Apr 13, 2022
@NikhilSharmaWe

This comment was marked as resolved.

Sign in to view
camilamacedo86
camilamacedo86 requested changes Apr 18, 2022
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your contribution. 🥇
But see that we need to fix the path: #2608 (comment)

@NikhilSharmaWe

This comment was marked as resolved.

Sign in to view
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2022
camilamacedo86
camilamacedo86 approved these changes Apr 22, 2022
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for contribution 🥇

The change on this PR will result in skipping the following jobs when only docs files were changed.
Screenshot 2022-04-22 at 09 02 22

The main follow-ups that I can see here show to be we begin to skip the e2e tests done in the prow when only doc files are changed. So that, if the change is only on the docs we do not need to run all e2e tests unnecessarily.

Also, maybe improve the implementation as we spoke about for we have not the code duplicate in all GitHub Actions to do the skip but that seems a nit and I am ok with that being handled/improved afterwards as well.

Let's see what others think about.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2022
@camilamacedo86
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2022
@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 22, 2022
@NikhilSharmaWe NikhilSharmaWe mentioned this pull request Apr 29, 2022
Closed
everettraven
everettraven approved these changes May 9, 2022
View reviewed changes
Copy link
Contributor

@everettraven everettraven left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, everettraven, NikhilSharmaWe, rashmigottipati

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@camilamacedo86
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2022
@k8s-ci-robot k8s-ci-robot merged commit 479cde1 into kubernetes-sigs:master May 14, 2022
@everettraven everettraven mentioned this pull request Jun 24, 2022
Closed
9 tasks
@NikhilSharmaWe NikhilSharmaWe mentioned this pull request Apr 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rashmigottipati rashmigottipati rashmigottipati approved these changes

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@everettraven everettraven everettraven approved these changes

@joelanford joelanford Awaiting requested review from joelanford

@pwittrock pwittrock Awaiting requested review from pwittrock

Assignees

@camilamacedo86 camilamacedo86

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@NikhilSharmaWe @camilamacedo86 @k8s-ci-robot @rashmigottipati @everettraven @joelanford