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

AZ-219 Replaced statefull force push check when pull requests are merged #168

Conversation

Marcin-Radecki
Copy link
Contributor

@Marcin-Radecki Marcin-Radecki commented Jan 11, 2022

This PR replaces a stateful force push check when pull requests are merged with a stateless version bump check when a new commit is pushed to any PR with the target branch as main.

The motivation of this change is to remove the solution with git push --force, and yet still have most of its features:

  • developer synchronization: with git push --force it is for CI to decide to bump the last version on main to the next one, so developers do not need to track if the version on main changed meanwhile sb merged other PR. With the proposed solution we don't have this particular gain, however, it should be rare people won't figure out the version changed meanwhile,
  • automatic solution prevented people from forgetting to update the version - this still holds, you can't merge PR which has source code changes without version bumped in it,
  • commit atomicity, ie we want ideally to have the version bumped in the same commit as the change due to the current way of publishing AlephBFT to crates.io - this still holds,
  • additional gain to remove git push --force is we can enable main as protected branch in GitHub, and that prevents git push --force from outside as well as forces PR to be raised with given a configurable minimal amount of approvers (this works on our other aleph-node repo).

Once this PR is merged, the other one #165 would be declined (thanks for that work which influenced this PR).

Testing done:

to main with stateless check when a commit on PR to main is pushed.
@github-actions
Copy link

github-actions bot commented Jan 11, 2022

Please make sure the following happened

@bartoszjedrzejewski
Copy link

What if PR changes only files in github workflows and doesnt req bump in version? As I see it, now it will req the version to be bumped anyway. At least the workflow will be run unnecessarily.

@Marcin-Radecki
Copy link
Contributor Author

What if PR changes only files in github workflows and doesnt req bump in version? As I see it, now it will req the version to be bumped anyway. At least the workflow will be run unnecessarily.

If PR changes only GitHub workflows, so in particular it does not change any files in src/ folder, then this check would be skipped. We don't bump versions automatically now, it is for a developer to do it (or not in given circumstances, as per above). Agree workflow would be run anyway, but it's rather cheap ('just' git clone and simple O(1) checks? And I assume workflows are run in parallel, so the CI workflow is longer anyway, as clippy takes more time than introduced version check workflow.

Copy link
Contributor

@timorl timorl left a comment

Choose a reason for hiding this comment

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

Nice, much better than forcepushing.

.github/scripts/check-cargo-toml-version-bumped.sh Outdated Show resolved Hide resolved
.github/workflows/check-version-bumped.yml Outdated Show resolved Hide resolved
.github/workflows/publish-package.yml Show resolved Hide resolved
.github/workflows/publish-package.yml Outdated Show resolved Hide resolved
.github/workflows/publish-package.yml Outdated Show resolved Hide resolved
@bartoszjedrzejewski
Copy link

What if PR changes only files in github workflows and doesnt req bump in version? As I see it, now it will req the version to be bumped anyway. At least the workflow will be run unnecessarily.

If PR changes only GitHub workflows, so in particular it does not change any files in src/ folder, then this check would be skipped. We don't bump versions automatically now, it is for a developer to do it (or not in given circumstances, as per above). Agree workflow would be run anyway, but it's rather cheap ('just' git clone and simple O(1) checks? And I assume workflows are run in parallel, so the CI workflow is longer anyway, as clippy takes more time than introduced version check workflow.

ok, but why waste github actions seconds pool for nothing. You can use this: https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#example-ignoring-paths
It's not a nogo for me, but would be nice to have it implemented.

@Marcin-Radecki
Copy link
Contributor Author

What if PR changes only files in github workflows and doesnt req bump in version? As I see it, now it will req the version to be bumped anyway. At least the workflow will be run unnecessarily.

If PR changes only GitHub workflows, so in particular it does not change any files in src/ folder, then this check would be skipped. We don't bump versions automatically now, it is for a developer to do it (or not in given circumstances, as per above). Agree workflow would be run anyway, but it's rather cheap ('just' git clone and simple O(1) checks? And I assume workflows are run in parallel, so the CI workflow is longer anyway, as clippy takes more time than introduced version check workflow.

ok, but why waste github actions seconds pool for nothing. You can use this: https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#example-ignoring-paths It's not a nogo for me, but would be nice to have it implemented.

Let me try with those paths, agreed if we can make it work so workflow are not spawned for nothing then it's better.

Copy link
Contributor

@kostekIV kostekIV left a comment

Choose a reason for hiding this comment

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

lgtm :)

output, also restricting workflow triggers for only certain path that
would save Github resources.
@Marcin-Radecki
Copy link
Contributor Author

Marcin-Radecki commented Jan 12, 2022

I've applied review remarks and it's better now, apart from cleaner bash we don't waste Guthub resources, and it's cleaner to developers than checks are run only if they modify particular files.

Testing done:

@Marcin-Radecki
Copy link
Contributor Author

In particular, you can see that two new checks are in place and triggered, since this PR modifies both src/ path and Cargo.toml:

image

@Marcin-Radecki Marcin-Radecki requested a review from timorl January 12, 2022 15:13
Copy link
Contributor

@timorl timorl left a comment

Choose a reason for hiding this comment

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

<3

@Marcin-Radecki Marcin-Radecki merged commit ae1d05a into Cardinal-Cryptography:main Jan 13, 2022
@Marcin-Radecki
Copy link
Contributor Author

Everything worked as expected after merge:

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