[Task #736] Add PR workflow to README #152

AnaCepic · 2025-01-08T08:01:34Z

Task: #736

Aim

Extract the NLBL workflow, combine with the survey results and add it to default rails template

Solution

template.rb

nikajukic · 2025-01-13T06:57:23Z

template.rb

+Merge-squash {feature-branch}
+
+(pull request {pull-request-link})
+[optionally](cherry picked from {commit-sha})


@uncoverd do you think we should add a bin/cherry-pick-squash (or some other name) script to this repository as well? https://app.productive.io/1-infinum/pages/148751

nikajukic · 2025-01-13T06:58:33Z

template.rb

+the pull request, when was the branch deployed to staging.
+
+Run the specs, check if everything is ok, then push 🎉
+


I'd add a placeholder here for other environments. E.g. some projects have UAT env, so I'd add something like

[TODO] if there is another environment present on the project, define the merge methodology for it.

nikajukic · 2025-01-13T07:03:02Z

template.rb

+Squash and merge way:
+
+Squash and merge combines all the commits from a feature branch into a single new commit on the master.
+The history of individual commits from the feature branch is not preserved in the master branch.
+
+While on a feature branch:
+
+```bash
+git fetch origin master
+git rebase -i origin/master
+git push --force
+```
+
+Check once again that everything was rebased correctly and continue on the master branch:
+
+```bash
+git fetch origin master
+git switch master && git pull
+git merge --squash {feature-branch}
+```
+
+Then commit the new changes with a message of this format:
+
+```bash
+{pr-title} ({pr-number})
+
+{pr-description}
+
+
+```
+
+*Note*: When doing merge-squash through terminal you will need to manually close the PR,
+as opposed to using the Squash and merge on GitHub


@uncoverd should we force to always use Github button for merging? Ideally we should have a rule in branch protection rules that no one should be able to push to master as everything should be done through PRs on Github. What do you think?

nikajukic · 2025-01-13T07:04:12Z

template.rb

+with `git merge --squash` (squash and merge button on GitHub) or non fast forward merge.
+Each of these is project specific and should be agreed upon which one would be used when starting the project.
+
+Once you agree you should update README accordingly.


I would somehow transform this into a [TODO] as ideally we'd keep only the information about the actual merge methodology in this Readme file. We can define both options in a Handbook, but only the relevant one should be kept in a readme.

vr4b4c · 2025-01-13T20:52:34Z

Honest question, shouldn't this be a link to our Handbook chapter?

AnaCepic · 2025-01-19T15:07:38Z

Honest question, shouldn't this be a link to our Handbook chapter?

@vr4b4c I agree that most of the content is similar to what we already have in our Handbook chapter. I think it would be better to do what Nika mentioned: replace some parts with [TODO].

I’m not sure if you’re questioning the entire idea of adding this to the README 😅, but I do believe we should include this in our default template, even if we already have something similar defined in the Handbook. 
From my experience, most developers tend to check the README file first when starting to work on a new project, rather than referring to the Handbook.
Having the workflow in the README also allows us to include project-specific details, such as environment-specific information, which can vary between projects.

That way we would have this information more accessible and it would also encourage people to clearly define these processes in the context of their project. This reduces the risk of important details being overlooked or vaguely considered.

vr4b4c · 2025-01-19T16:42:27Z

I’m not sure if you’re questioning the entire idea of adding this to the README 😅,

Correct, I'm questioning the whole idea of having the sizeable amount of content about workflow repeated in projects README. A link(s) to the relevant chapter in the Handbook would be a better approach. But I'm open-minded towards new ideas, feel free to challenge my opinions.

but I do believe we should include this in our default template, even if we already have something similar defined in the Handbook. From my experience, most developers tend to check the README file first when starting to work on a new project, rather than referring to the Handbook.

I'm not saying to omit the workflow documentation in the project repo, I'm saying we shouldn't repeat ourselves and link to some extent is more suitable for this use case (more on exact reasons follows). Let me give you some concrete examples of sections where I think link to the Handbook chapter is more suitable.

Commits

The commits section is very concise, generic (not project specific), and stable (not likely to change existing practices, albeit might be extended with some new ones). Keeping it centralized in the Handbook chapter would make it easier to manage and update. Since it's centralized, information propagation is free. I don't think it's likely for something project-specific to come up in this context, but if it does, the link would still be valid and the project would document only deviations from the centralized knowledge base.

Branches

The same as for commits, handbook chapter.

Pull Requests

The same as for commits and branches, handbook chapter, which covers much more than the section in the default template.

Solving Change Requests

This is best practice advice, I don't think it's likely to change on a per-project basis (handbook chapter).

Merge methodology

I do see this as something that's more likely to change than any other previously mentioned section. Still, I think it would be better to link it to Handbook chapter and document only the differences to the established practice.

Having the workflow in the README also allows us to include project-specific details, such as environment-specific information, which can vary between projects.

Linking generic topics shouldn't (and doesn't in my opinion) hinder our ability to document project-specific details.

That way we would have this information more accessible and it would also encourage people to clearly define these processes in the context of their project. This reduces the risk of important details being overlooked or vaguely considered.

I don't think there is any difference regarding this concern if we link content or not. Depending on the change affiliation it should be added in the project repo (if it's project-specific) or changed in the Handbook chapter if it's globally applicable.

I'm now going to add a few comments (which should be fixed in the Handbook chapter), and think about how difficult would it be to propagate those changes if this PR was merged as is and applied to a dozen projects.

vr4b4c · 2025-01-19T16:43:10Z

template.rb

+Commits should have a descriptive subject as well as a quick explanation of the reason for the change in the commit
+body.
+This makes it easier to check changes in the code editor as you do not have to find the pull request and open it
+on github.


Suggested change

on github.

on GitHub.

vr4b4c · 2025-01-19T16:43:34Z

template.rb

+
+### Branches
+
+Branches should be opened from the master branch. Naming convention is {type}/{task-number}{descriptive-task-name}


Suggested change

Branches should be opened from the master branch. Naming convention is {type}/{task-number}{descriptive-task-name}

Branches should be opened from the main branch. Naming convention is {type}/{task-number}{descriptive-task-name}

vr4b4c · 2025-01-19T16:47:36Z

template.rb

+
+### Branches
+
+Branches should be opened from the master branch. Naming convention is {type}/{task-number}{descriptive-task-name}


Using folder-like structure in the branch names makes it more difficult to adopt git worktree's in practice. I'd personally suggest sticking to {task-number}-{descriptive-task-name} naming convention (this also matches naming strategy of task management systems that integrate with source control systems).

vr4b4c · 2025-01-19T16:48:45Z

template.rb

+#### Solving Change Requests
+
+Change requests should be fixed in separate fixup or squash commits. Rebasing the branch during an ongoing review is not
+appreciated unless there is a good reason for it, like pulling in some new and necessary changes from master, because it


Suggested change

appreciated unless there is a good reason for it, like pulling in some new and necessary changes from master, because it

appreciated unless there is a good reason for it, like pulling in some new and necessary changes from the main branch, because it

vr4b4c · 2025-01-19T16:50:03Z

template.rb

+
+### Staging
+
+Once the PR is opened, the feature can be merged into staging,


Suggested change

Once the PR is opened, the feature can be merged into staging,

Once the PR is opened, the feature can be integrated into staging,

vr4b4c · 2025-01-19T16:52:56Z

template.rb

+
+##### Merge Methodology
+
+We are doing merge-squashes to staging, as well of resets of the staging branch to master after each


Suggested change

We are doing merge-squashes to staging, as well of resets of the staging branch to master after each

We are doing merge-squashes to staging, as well of resets of the staging branch to the main branch after each

vr4b4c · 2025-01-19T16:53:27Z

template.rb

+[optionally](cherry picked from {commit-sha})
+```
+
+Including the pull request link makes github pick up that commit in the pull request, so we can know directly in


Suggested change

Including the pull request link makes github pick up that commit in the pull request, so we can know directly in

Including the pull request link makes GitHub pick up that commit in the pull request, so we can know directly in

vr4b4c · 2025-01-19T16:54:01Z

template.rb

+
+[TODO] If there is another environment present on the project, define the merge methodology for it.
+
+### Master (Production)


Suggested change

### Master (Production)

### Main branch (Production)

vr4b4c · 2025-01-19T16:54:11Z

template.rb

+### Master (Production)
+
+Once the PR has at least 1 approval, the branch was successfully deployed to staging, tested,
+and there are no failing specs, it can be merged into master.


Suggested change

and there are no failing specs, it can be merged into master.

and there are no failing specs, it can be merged into the main branch.

vr4b4c · 2025-01-19T16:55:29Z

template.rb

+Once the PR has at least 1 approval, the branch was successfully deployed to staging, tested,
+and there are no failing specs, it can be merged into master.
+
+##### Merge methodology


Suggested change

##### Merge methodology

##### Integration methodology

Add PR workflow to README

2572af4

AnaCepic self-assigned this Jan 8, 2025

AnaCepic requested review from nikajukic, cilim, uncoverd and vr4b4c as code owners January 8, 2025 08:01

nikajukic reviewed Jan 13, 2025

View reviewed changes

Replace some parts with TODOs

015271c

AnaCepic requested a review from nikajukic January 19, 2025 15:09

vr4b4c requested changes Jan 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Task #736] Add PR workflow to README #152

[Task #736] Add PR workflow to README #152

AnaCepic commented Jan 8, 2025

nikajukic Jan 13, 2025

nikajukic Jan 13, 2025

nikajukic Jan 13, 2025

nikajukic Jan 13, 2025

vr4b4c commented Jan 13, 2025

AnaCepic commented Jan 19, 2025

vr4b4c commented Jan 19, 2025

vr4b4c Jan 19, 2025

vr4b4c Jan 19, 2025

vr4b4c Jan 19, 2025

vr4b4c Jan 19, 2025

vr4b4c Jan 19, 2025

vr4b4c Jan 19, 2025

vr4b4c Jan 19, 2025

vr4b4c Jan 19, 2025

vr4b4c Jan 19, 2025

vr4b4c Jan 19, 2025

		the pull request, when was the branch deployed to staging.

		Run the specs, check if everything is ok, then push 🎉


		### Branches

		Branches should be opened from the master branch. Naming convention is {type}/{task-number}{descriptive-task-name}

	Branches should be opened from the master branch. Naming convention is {type}/{task-number}{descriptive-task-name}
	Branches should be opened from the main branch. Naming convention is {type}/{task-number}{descriptive-task-name}

	appreciated unless there is a good reason for it, like pulling in some new and necessary changes from master, because it
	appreciated unless there is a good reason for it, like pulling in some new and necessary changes from the main branch, because it


		### Staging

		Once the PR is opened, the feature can be merged into staging,


		##### Merge Methodology

		We are doing merge-squashes to staging, as well of resets of the staging branch to master after each

	Including the pull request link makes github pick up that commit in the pull request, so we can know directly in
	Including the pull request link makes GitHub pick up that commit in the pull request, so we can know directly in


		[TODO] If there is another environment present on the project, define the merge methodology for it.

		### Master (Production)

	and there are no failing specs, it can be merged into master.
	and there are no failing specs, it can be merged into the main branch.

[Task #736] Add PR workflow to README #152

Are you sure you want to change the base?

[Task #736] Add PR workflow to README #152

Conversation

AnaCepic commented Jan 8, 2025

Aim

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vr4b4c commented Jan 13, 2025

AnaCepic commented Jan 19, 2025

vr4b4c commented Jan 19, 2025

Commits

Branches

Pull Requests

Solving Change Requests

Merge methodology

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment