Skip to content

Remove git push commands after creating a new commit or branch #472

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

Merged
merged 7 commits into from
May 12, 2023

Conversation

oguzkocer
Copy link
Contributor

@oguzkocer oguzkocer commented May 9, 2023

Closes #308.

What does it do?

This PR removes the git push commands after creating a new commit or a branch. The goal for this change is to leave the decision for pushing the changes to remote to the developer or the project configuration.

For projects where a developer manages the release in their local environment, this gives them the flexibility to check the diff before pushing. It also removes the inconsistency where some actions were pushing the changes to remote and others weren't. A minor drawback that needs to be taken into account is that we can't trigger a beta/final build in CI before we push the change. So, we'd potentially need to add a push command to the project's Fastfile before these triggers.

For projects where the release is managed in CI, this allows us to decide the best time to push the changes to remote which in most cases should be after we are done with all the necessary changes. If we only push the changes when everything is successful, there will be less changes to revert if something goes wrong. Note that there might still be changes that needs to be reverted, such as closing a milestone, because these actions are not related to git commits. However, with release management in CI, we'll likely not need to worry about these either.


The code changes in this PR are very straightforward. I removed all the push commands, the arguments them and any mention of it in the documentation. Since this is a breaking change which requires extra steps, I've also added a new MIGRATION.md file which has steps to migrate release-toolkit from 7.0.0 to 8.0.0.


Note that the only remaining push command is in create_tag. I don't know if this action makes much sense without pushing to remote, because we don't really care about local tags. On the bright side, I don't think the actions that use this helper is being used at the moment. #471 proposes the deprecation of ios_final_tag & ios_clear_intermediate_tags actions and I intend to follow that up with another PR that proposes the deprecation of android_tag_build. This is because we no longer tag our builds and instead create a draft GitHub release which upon publishing will create the tag for us. So, we'll likely end up removing the create_tag helper as well.

Checklist before requesting a review

  • Run bundle exec rubocop to test for code style violations and recommendations
  • Add Unit Tests (aka specs/*_spec.rb) if applicable
  • Run bundle exec rspec to run the whole test suite and ensure all your tests pass
  • Make sure you added an entry in the CHANGELOG.md file to describe your changes under the approprioate existing ### subsection of the existing ## Trunk section.

@oguzkocer oguzkocer marked this pull request as ready for review May 9, 2023 02:07
@oguzkocer oguzkocer enabled auto-merge May 9, 2023 02:09
@oguzkocer oguzkocer disabled auto-merge May 9, 2023 02:10
@oguzkocer oguzkocer requested a review from a team May 9, 2023 02:10

We are no longer pushing to remote after creating a new commit or a branch. That means, developers need to manually push the changes or add push commands in the project's `Fastfile`. Most importantly, we can no longer immediately trigger beta/final builds after creating a new commit because the changes will not be in remote yet. If you want to keep the existing behavior, you'll need to add a push command before these triggers.

For example, in [WordPress-Android's `new_beta_release` lane](https://github.com/wordpress-mobile/WordPress-Android/blob/0c64cb84c256e004473e97d72b4ac6682ebc140b/fastlane/lanes/release.rb#L86), we download translations, bump the beta version and then trigger a new build in CI. After migrating to `8.0.0` of `release-toolkit`, we'll need to add [`push_to_git_remote`](https://docs.fastlane.tools/actions/push_to_git_remote/) command before this trigger to keep the existing behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, we discussed having a MIGRATION.md document during @iangmaia's trial, in #452

My understanding of the conclusion reached at the time was to put the information into the CHANGELOG.md to avoid having multiple file to keep track of and update.

It's a "tough" call. On the one hand, it's neat to move the info here to avoid cluttering the changelog. On the other, separating information might make it less discoverable. ⚖️

For what is worth, since you already implemented this I'm happy to keep it. It will be easy enough to move the content into CHANGELOG.md later on if we find it cumbersome or lossy to maintain both files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @mokagio! I missed this discussion.

I like having a separate file - as is probably clear from my proposal in the first place, but I am happy to move the instructions to CHANGELOG file. I just need some guidance on where exactly I should put it.

@oguzkocer oguzkocer requested a review from a team May 9, 2023 03:09
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

Searched for the word push in the code and confirmed that all the remaining occurrences found are not related to git push, so seems that all of them have been removed.

MIGRATION.md file is also clear 👍

:shipit:

@oguzkocer oguzkocer merged commit e74576a into trunk May 12, 2023
@oguzkocer oguzkocer deleted the remove/git-push-actions branch May 12, 2023 01:14
@oguzkocer
Copy link
Contributor Author

Thank you for the review @AliSoftware! 🙇‍♂️

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.

Don't auto-push from actions
3 participants