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

Workflows are improved #1318

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Workflows are improved #1318

merged 4 commits into from
Aug 12, 2024

Conversation

erichaagdev
Copy link
Member

This PR contains several very minor fixes & improvements to the workflows in this repository. See each commit for details.

@erichaagdev erichaagdev requested review from jprinet and a team August 2, 2024 18:07
@erichaagdev erichaagdev self-assigned this Aug 2, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

Job Requested goals Build Tool Version Build Outcome Build Scan®
verification-[object Object] wrapper:wrapper 3.9.8 Build Scan NOT_PUBLISHED
verification-[object Object] clean verify 3.8.8 Build Scan PUBLISHED
Generated by gradle/develocity-actions

Copy link
Member

@jprinet jprinet left a comment

Choose a reason for hiding this comment

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

LGTM although I feel we should relax a bit the run condition

@@ -3,10 +3,10 @@ name: Verify Convention Develocity Gradle Plugin
on:
push:
branches: [ main ]
paths: [ 'convention-develocity-gradle-plugin/**' ]
paths: [ 'convention-develocity-gradle-plugin/**', '.github/workflows/convention-develocity-gradle-plugin-verification.yml' ]
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we should be too fine grained here, as a rename is likely to remain unnoticed.
I'd rather go with prefixed patterns

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, but workflows are unlikely to change often and when they do, the impact of missing this is minimal. How would you configure the prefixed patterns?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could afford '.github/workflows/**'

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Jerome: making it simpler probably outweighs any advantage to only running the changed workflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know if GitHub exposes a variable for "path to current workflow" and if it could be used within an on.push.paths: condition? I'd really like to avoid running all workflows with .github/workflows/** when any workflow has changed. It would cause an explosion of unnecessary workflow runs which is where we used to be before adding these on: conditions in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

@erichaagdev - I can't link as directly to it as I would like, but I see a github.workflow_ref that contains (as part of a longer string) the workflow file name: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/contexts#github-context

Would this solve your problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. We need the path to the workflow file exactly. Even if we had that though, I'm not sure it would work. Will need to test it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I would lean towards Jerome's solution above (.github/workflows/**).

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in b215730

@erichaagdev erichaagdev force-pushed the erichaagdev/workflow-improvements branch from 995e515 to 35a2646 Compare August 9, 2024 19:54
@erichaagdev erichaagdev merged commit c07871d into main Aug 12, 2024
32 checks passed
@erichaagdev erichaagdev deleted the erichaagdev/workflow-improvements branch August 12, 2024 14:37
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.

4 participants