Skip to content

Conversation

@zachschuermann
Copy link
Member

@zachschuermann zachschuermann commented Nov 19, 2024

skip running our CI for CHANGELOG.md. this will become more useful as we move to a new unreleased section in the changelog which PR authors will add to :)

and remove edited PR trigger for semver checks

Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

ohh nice, glad this is easy

@codecov
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@ccf5e9a). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #511   +/-   ##
=======================================
  Coverage        ?   80.21%           
=======================================
  Files           ?       61           
  Lines           ?    13342           
  Branches        ?    13342           
=======================================
  Hits            ?    10702           
  Misses          ?     2092           
  Partials        ?      548           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zachschuermann
Copy link
Member Author

@nicklan did we previously learn that this needs to merge before the changes will propagate to CI configuration?

@nicklan
Copy link
Collaborator

nicklan commented Nov 19, 2024

@nicklan did we previously learn that this needs to merge before the changes will propagate to CI configuration?

No, they should take effect here. It's just permissions that only apply against main

@zachschuermann
Copy link
Member Author

hmm it looks like it doesn't trigger but it invalidated the previous status for all the required checks....?

@nicklan
Copy link
Collaborator

nicklan commented Nov 19, 2024

hmm it looks like it doesn't trigger but it invalidated the previous status for all the required checks....?

Try fixing the conflicts and see what happens

Copy link
Collaborator

@OussamaSaoudi-db OussamaSaoudi-db left a comment

Choose a reason for hiding this comment

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

Nice! Just don't forget to remove the test in CHANGELOG :)

push:
paths-ignore:
- 'CHANGELOG.md'
pull_request:
Copy link
Collaborator

Choose a reason for hiding this comment

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

While we're at it, can we consider using types to prevent CI from triggering when the PR description changes? The documentation states that

By default, a workflow only runs when a pull_request event's activity type is opened, synchronize, or reopened. To trigger workflows by different activity types, use the types keyword.

I believe we're interested in not triggering on the edited activity type, which would have been manually added somewhere, according to the above?

Copy link
Member Author

Choose a reason for hiding this comment

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

nice good find - ill edit!

Copy link
Member Author

Choose a reason for hiding this comment

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

pull_request_target:
types:
- opened
- edited
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's the culprit!

Also, shouldn't we be triggering semver on push rather than (only) on pull_request_target?

Copy link
Member Author

Choose a reason for hiding this comment

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

nice thanks! and yea I think we want both - fixing :)

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @nicklan in case im missing anything?

@zachschuermann
Copy link
Member Author

i hate to be the bearer of bad news but it seems this isn't working? looks like all the CI still ran on that last commit that only changes CHANGELOG

@@ -1,5 +1,5 @@
# Changelog

test
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert before merging?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep - still debugging since it ran all the CI despite the new ignore-paths 😞

@zachschuermann
Copy link
Member Author

turns out github detects changes based on the entire diff of the PR not each commit so this becomes significantly harder now.. closing this and will open a follow-up that does some minimal CI improvements

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