AZ 219 remove git force push to main by KazyCC · Pull Request #165 · Cardinal-Cryptography/AlephBFT

KazyCC · 2021-12-15T14:34:33Z

Changes:

in push-foundation-repo.yml automatic publish to crates.io will be disabled for now.
removed force-push
version is updating on PR branch, not main
new version is established before completing the PR

…s: 1) "prepare-new-version" triggereed on PR (open, reopen, synchronize); 2) "autobump" publishing new version to crates.io during push into main.

…pushes.

github-actions · 2021-12-15T14:34:49Z

Please make sure the following happened

Appropriate tests created
Infrastructure updated accordingly
Updated existing documentation
New documentation created
Version bumped if breaking changes

KazyCC · 2021-12-15T14:39:15Z

It looks like there is an issue with permissions - https://github.com/Cardinal-Cryptography/AlephBFT/actions/runs/1583186193? @bartoszjedrzejewski could you assist?

timorl

A couple nits, some naming confusion and one question. Looks nice in general!

.github/workflows/push-foundation-repo.yml

timorl · 2021-12-15T14:47:25Z

.github/workflows/push-foundation-repo.yml

+#    workflows: ["Autobump version"]
+#    branches: [main]
+#    types:
+#      - completed


Commented code.

By purpose - see note above.

That's why we have git though – if someone wants to see what once was here they can use history. Commented code is just confusing too often.

It's not about history - commented code is left there, because it is the right code to be enabled in the future, when workflows stabilize.

Yeah, but keeping commented code is always confusing. I would rather trust the person who will enable the automation here to do this correctly. If they need help one of the options is to look in the history, that's what I meant by it staying there.

.github/workflows/prepare-version-bump.yml

timorl · 2021-12-15T14:50:56Z

.github/workflows/version-bump.yml

-          if [ -f publishMe ]; then
-            cargo publish
-          fi
+          echo 'Uploading to crates.io'


Uh, I'm confused now, this always pushes to crates.io?

Anyway, you probably want to rename this file? It seems there was some naming confusion here.

This workflow is invoked right now only manually, so yes, if you invoke it manually, it will publish every time it will be called.

I'm confused, although maybe it's just my lack of understanding of workflows – this file seems to imply it will always run on push to main, the other file states it only runs manually. And the other one also publishes. Shouldn't this file just be deleted then?

Ok, I did some clean up. See 4339e93. Workflow should be like this:

On PR creation to main -> prepare-version-bump.yml

Manually (for now) -> version-bump.yml

On (2) -> push-foundation-repo.yml.

Yeah, that seems good now. I still have some gripes with naming – maybe call this one something like "Publish to crates.io" rather than "Autobump version"? Similar for file names, what is now called "prepare-..." should just be "version-bump" and this one "publish" or something.

timorl · 2021-12-15T16:26:16Z

.github/workflows/push-foundation-repo.yml

+#    workflows: ["Autobump version"]
+#    branches: [main]
+#    types:
+#      - completed


That's why we have git though – if someone wants to see what once was here they can use history. Commented code is just confusing too often.

timorl · 2021-12-15T16:30:06Z

.github/workflows/version-bump.yml

-          if [ -f publishMe ]; then
-            cargo publish
-          fi
+          echo 'Uploading to crates.io'


I'm confused, although maybe it's just my lack of understanding of workflows – this file seems to imply it will always run on push to main, the other file states it only runs manually. And the other one also publishes. Shouldn't this file just be deleted then?

.github/workflows/prepare-version-bump.yml

timorl

Still some minor gripes. ^^"

timorl · 2021-12-16T10:38:45Z

.github/workflows/version-bump.yml

-          if [ -f publishMe ]; then
-            cargo publish
-          fi
+          echo 'Uploading to crates.io'


Yeah, that seems good now. I still have some gripes with naming – maybe call this one something like "Publish to crates.io" rather than "Autobump version"? Similar for file names, what is now called "prepare-..." should just be "version-bump" and this one "publish" or something.

timorl · 2021-12-16T10:41:39Z

.github/workflows/push-foundation-repo.yml

+#    workflows: ["Autobump version"]
+#    branches: [main]
+#    types:
+#      - completed


Yeah, but keeping commented code is always confusing. I would rather trust the person who will enable the automation here to do this correctly. If they need help one of the options is to look in the history, that's what I meant by it staying there.

timorl · 2021-12-16T10:42:33Z

.github/workflows/push-foundation-repo.yml

@@ -1,4 +1,4 @@
-name: Sync Cardinal-Cryptography repo with Aleph-Zero-Foundation repo
+name: Sync Cardinal-Cryptography repo with Aleph-Zero-Foundation repo.


Nit: All the other names don't have punctuation, please make this consistent (whichever way you prefer).

kostekIV · 2021-12-20T10:12:09Z

.github/workflows/prepare-version-bump.yml

+          ref: ${{ github.head_ref }}
+          token: ${{ secrets.SYNCAZF || secrets.MY_TOKEN || github.token }}
+      - name: check-and-bump
+        run: |


nit: this block can be made into sh script (and put somewhere in the scripts/... folder). This way it will be more readable here and easier to review the script without the noise from the workflow file.

Marcin-Radecki · 2022-01-13T09:55:05Z

I am closing this PR on behalf we've incorporated its inputs in #168 .

Thanks for work on this :)

KazyCC and others added 20 commits December 15, 2021 15:06

Refactoring version bumping

4bf02ae

Splitting version-bump job triggered on "push" to "main" into two job…

402b6b3

…s: 1) "prepare-new-version" triggereed on PR (open, reopen, synchronize); 2) "autobump" publishing new version to crates.io during push into main.

AZ-219 splitting autobum into two files, in order to not using force …

7e03172

…pushes.

Temporary adding test_main branch handling for testing

753cef5

Removing cargo publish from test_main branch testing

0d4ff1b

Changing creates publish into manual workflow_dispatch

1f9b007

Simplfing the push command

5ae61db

Prcising the checkout

f6eb26d

Working on tokens for the checkout

de09bcf

Working on tokens for the checkout (2)

91e51be

Cleanup of both version prep and autobump

9b29dad

Cleanup of both version prep and autobump (2)

85fde32

Switch to apply version of PR branch, not target branch.

36e6263

Adding tags pushing

4ae86fa

Adding tags pushing

a436772

Adding check to do not try to create duplicate tags.

624aad4

Setting version to 0.8.3

c158f4a

Cleanup before PR

56c0308

Renaming

708aa43

Cargo.lock version update

b8df6b4

KazyCC requested review from bartoszjedrzejewski and timorl December 15, 2021 14:34

timorl suggested changes Dec 15, 2021

View reviewed changes

Improving name to clearly state purpose.

dc09229

timorl suggested changes Dec 15, 2021

View reviewed changes

Resolving PR comments

4339e93

timorl suggested changes Dec 16, 2021

View reviewed changes

kostekIV reviewed Dec 20, 2021

View reviewed changes

Marcin-Radecki mentioned this pull request Jan 11, 2022

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

Merged

Marcin-Radecki closed this Jan 13, 2022

		@@ -1,4 +1,4 @@
		name: Sync Cardinal-Cryptography repo with Aleph-Zero-Foundation repo
		name: Sync Cardinal-Cryptography repo with Aleph-Zero-Foundation repo.

Conversation

KazyCC commented Dec 15, 2021

Uh oh!

github-actions bot commented Dec 15, 2021 • edited by KazyCC Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KazyCC commented Dec 15, 2021

Uh oh!

timorl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

timorl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcin-Radecki commented Jan 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Dec 15, 2021 •

edited by KazyCC

Loading