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

ci: disable stages if only docs changes or .github #2877

Closed
wants to merge 3 commits into from

Conversation

v1v
Copy link
Member

@v1v v1v commented Mar 23, 2022

What does this PR do?

Faster builds for changes which are not verified/validated by the build system

Why

Smart builds reduce the cost (money and time) for commits which don't need to be validated

Therefore, #2874 could be a good candidate to be done in a few minutes rather than dozens of minutes.

@v1v v1v added Team:Automation Label for the Observability productivity team automation labels Mar 23, 2022
@v1v v1v requested a review from a team March 23, 2022 12:49
@v1v v1v requested a review from a team as a code owner March 23, 2022 12:49
@v1v v1v self-assigned this Mar 23, 2022
@elasticmachine
Copy link

elasticmachine commented Mar 23, 2022

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-03-23T13:37:42.388+0000

  • Duration: 96 min 58 sec

Test stats 🧪

Test Results
Failed 0
Passed 4210
Skipped 7
Total 4217

Steps errors 1

Expand to view the steps failures

Test integration: zeek
  • Took 65 min 56 sec . View more details here
  • Description: eval "$(../../build/elastic-package stack shellinit)" ../../build/elastic-package test -v --report-format xUnit --report-output file --test-coverage

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks! Added a couple of comments.

.ci/Jenkinsfile Outdated
@@ -41,9 +41,16 @@ pipeline {
deleteDir()
gitCheckout(basedir: "${BASE_DIR}")
stashV2(name: 'source', bucket: "${JOB_GCS_BUCKET}", credentialsId: "${JOB_GCS_CREDENTIALS}")
dir("${BASE_DIR}"){
// Skip all the stages except check Go sources for changeset's with asciidoc, md or .github changes only
setEnvVar('ONLY_DOCS', isGitRegionMatch(patterns: [ '(.*\\.(asciidoc|md)|^\\.github/.*)' ], shouldMatchAll: true).toString())
Copy link
Member

Choose a reason for hiding this comment

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

We want to run mage check if .github/CODEOWNERS change (see #2768). If this is a problem we could extract this to a different target that is always run, though mage check is fast now.

Comment on lines +64 to +66
when {
expression { return env.ONLY_DOCS == "false" }
}
Copy link
Member

Choose a reason for hiding this comment

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

Packages have generated content in their README.md files, elastic-package lint or elastic-package check should be executed if doc files are changed, to ensure that the generated content hasn't been unexpectedly modified.

Maybe instead of having the condition here for the whole stage, it could be after the sh that runs elastic-package check below.

@mtojek mtojek self-requested a review March 23, 2022 13:33
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Hi, did we analyze the current costs of running? What we have right now is spawning nodes only for the affected integrations, which I would consider as a required minimum. If we want to optimize it further, I'd like to talk and identify first the most expensive components, instead of sneaking conditions.

@v1v
Copy link
Member Author

v1v commented Mar 23, 2022

setEnvVar('ONLY_DOCS', isGitRegionMatch(patterns: [ '(^docs/.*\\.md|^\\.github/.*)' ], shouldMatchAll: true).toString()) is the approach to skip everything if changes only in .github and docs.

@v1v
Copy link
Member Author

v1v commented Mar 23, 2022

did we analyze the current costs of running?

We have found this particular behaviour in addition to #2878 and also the missing tear down in elastic/elastic-package#701

As a consequence, I looked at the pipeline and raised two different PRs to reduce the number of builds in the CI for changes that potentially are not needed.

@mtojek
Copy link
Contributor

mtojek commented Mar 23, 2022

Maybe we should run different pipelines for different elements? Like "docs pipeline", "integrations pipeline", etc.

I agree we don't need to spawn a hundred nodes to check docs, but the last time we changed docs was 9 days ago, 2 months ago, 12 months ago.

@v1v
Copy link
Member Author

v1v commented Mar 23, 2022

To clarify, I didn't use any data in terms of justifying this change is needed, but found out that for some particular PRs won't need to run all the stages as those stages do not test anything for those changes. So, filtering what to run and when could help regardless if it's often changed or not. Even if it's not, there is a chance that accidentally someone could create a few PRs and then cope with all the CI capacity for instance :/

@v1v
Copy link
Member Author

v1v commented Mar 23, 2022

Maybe we should run different pipelines for different elements? Like "docs pipeline", "integrations pipeline", etc.

No strong opinion, even by doing so, it's required to create the filtering. So adding the filtering at the stage level reduces the number of GitHub api calls.

@v1v v1v closed this Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation Team:Automation Label for the Observability productivity team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants