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: don't skip the ci when we have automatic commits #8089

Merged
merged 1 commit into from Apr 4, 2022
Merged

Conversation

domoritz
Copy link
Member

@domoritz domoritz commented Apr 4, 2022

Otherwise we skip CI for commits coming from the CI: #8088.

📦 Published PR as canary version: 5.2.1--canary.8089.0345f00.0

✨ Test out this PR locally via:

npm install vega-lite@5.2.1--canary.8089.0345f00.0
# or 
yarn add vega-lite@5.2.1--canary.8089.0345f00.0

Copy link
Member

@arvind arvind left a comment

Choose a reason for hiding this comment

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

I think this is right, but I'll defer to @hydrosquall.

Copy link
Member

@hydrosquall hydrosquall left a comment

Choose a reason for hiding this comment

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

Thinking through the logic of why CI skip was present in the past

  1. [ci skip] is put in place to avoid a situation where you have an infinite loop (github actions repeatedly spawning more github actions)
  2. These specific situations are OK to not CI skip because they're guarded by branches that should not be re-entered after the contents of those branches have been run (specifically git status --porcelain and git diff

With these two reasoning steps in mind, I'm reasonably confident that "skipping the skips" won't lead to an infinite chain of commits being made, so it's safe to change our commit messaging scheme.

@domoritz
Copy link
Member Author

domoritz commented Apr 4, 2022

Same. I think we could add a safeguard where ci commits don't spawn new commits but I think these should be safe so let's merge.

@domoritz domoritz merged commit ba6b9ca into next Apr 4, 2022
@domoritz domoritz deleted the dom/ci-skip branch April 4, 2022 21:51
@github-actions
Copy link

github-actions bot commented Apr 7, 2022

🚀 PR was released in v5.3.0-next.2 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants