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

run workflow only if modified file matches the configured paths #4919

Merged
merged 1 commit into from
Dec 23, 2021

Conversation

marulitua
Copy link
Contributor

@marulitua marulitua commented Oct 23, 2021

Q A
Type improvement
BC Break no
Fixed issues #4854

Summary

static analysis:

  • src/
  • static-analysis/
  • tests/

continuous integration:

  • src/
  • tests/
  • ci/
  • composer.json
  • composer.lock

coding standards:

  • src/
  • bin/
  • tests/

@derrabus derrabus added the CI label Oct 23, 2021
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

A good way to validate the completeness of the paths might be to use strace, run the jobs locally (each tool once) and see what files they actually use or even attempt to use. For instance:

$ strace cat test.php 2>&1 | grep openat
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/lib/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/usr/lib/locale/locale-archive", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "test.php", O_RDONLY)  = 3

Then disregard all the paths that don't belong to the project. Here, we can see that cat test.php depends on test.php.

.github/workflows/coding-standards.yml Outdated Show resolved Hide resolved
paths:
- "src/**"
- "bin/**"
- "tests/**"
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to get rid of the duplication of the paths for each workflow?

Copy link
Member

Choose a reason for hiding this comment

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

@marulitua please do not resolve the conversations that you didn't start. What's the resolution on this one?

.github/workflows/static-analysis.yml Outdated Show resolved Hide resolved
@derrabus derrabus added this to the 2.13.5 milestone Oct 23, 2021
@marulitua
Copy link
Contributor Author

@morozov Can you check on my update

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

Content-wise looks good. Please retarget against 2.13.x since it's relevant there as well. You'll have to replace src/ with lib/ there.

paths:
- "src/**"
- "bin/**"
- "tests/**"
Copy link
Member

Choose a reason for hiding this comment

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

@marulitua please do not resolve the conversations that you didn't start. What's the resolution on this one?

.github/workflows/static-analysis.yml Outdated Show resolved Hide resolved
.github/workflows/coding-standards.yml Outdated Show resolved Hide resolved
@marulitua marulitua changed the base branch from 3.1.x to 2.13.x October 28, 2021 00:51
@morozov
Copy link
Member

morozov commented Oct 28, 2021

Hmm… it looks like required jobs remain required even if they are skipped. Now I see that in Debezium where I spotted this approach, they don't mark any of the jobs as required. @greg0ire do you think it will be safe to leave the status check to the maintainer's discretion?

Another thing is that the AppVeyor build still runs unconditionally. It would be a bummer not to optimize it as well since it's one of the slowest jobs (especially, in the older branches like 2.13.x).

@greg0ire
Copy link
Member

@greg0ire do you think it will be safe to leave the status check to the maintainer's discretion?

If we have to choose between both features I think it's best to pick the paths one. I'm sure Github will come back with a better solution in the future. It would be great to report this though.

@morozov
Copy link
Member

morozov commented Oct 28, 2021

It would be great to report this though.

Agree. Do you know where this could be reported?

@morozov
Copy link
Member

morozov commented Oct 28, 2021

It should be possible to apply this to AppVeyor as well (reference).

@greg0ire
Copy link
Member

Agree. Do you know where this could be reported?

Well, there is this feedback form, and otherwise you have https://github.community/ which will provide workarounds . Neither will feel like a great way of reporting this I'm afraid.

@morozov
Copy link
Member

morozov commented Oct 29, 2021

Submitted to both (ref).

@derrabus derrabus modified the milestones: 2.13.5, 2.13.6 Nov 11, 2021
@stof
Copy link
Member

stof commented Nov 15, 2021

this misses one path: the workflow file itself.

@greg0ire greg0ire modified the milestones: 2.13.6, 2.13.7 Nov 26, 2021
@morozov morozov linked an issue Dec 23, 2021 that may be closed by this pull request
@morozov morozov merged commit f97a4bc into doctrine:2.13.x Dec 23, 2021
@morozov
Copy link
Member

morozov commented Dec 23, 2021

I believe we can tackle the AppVeyor part later. Thanks, @marulitua!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize build pipeline by taking affected paths into account
5 participants