Support proper trunk flow #675

gitstart-app · 2024-11-12T06:16:08Z

Closes #548

Add trunk branching strategy

Loom

https://www.loom.com/share/7390db10eb874dca8a29f30ec92a0c34?sid=bf1a52d2-af90-4b28-9a4e-bde5609b5b5b

gitstart-tramline · 2024-11-12T12:18:49Z

PR is still in progress and not ready for review yet

gitstart-tramline · 2024-11-20T16:19:40Z

Hi @kitallis, PR is ready for review. Please review.

kitallis

Thanks for the PR. Initial comments, before I review the code. I tested it out and there's a few functional issues:

As soon as we start the release with Trunk, we should trigger the release for the HEAD of the working branch and not wait for a commit to show up before starting. The flow for when a commit lands and needs to be applied through the queue is correct, but should only happen for HEAD+N commits
The workflow runs are not being correctly found in trunk mode, they only seem to be found when the tag is used instead of the release branch. I tested this. The culprit is here. Perhaps we should only look for it via the commit SHA? We'll have to test this for Bitbucket and GitLab as well (I haven't done so yet).
We should disable all automations at the end. This flow breaks currently when you reach the end of the release. The FinalizeRelease should early return perhaps.
We should not show this message in the finale phase and instead say something like the following in that card: Since we're doing a trunk-based branching strategy, there's no end-of-release housekeeping required on Tramline's end. We'll just refresh the DevOps reports and charts for you after the release is complete.

kitallis

I think there are a lot of structural issues with this, which I've called out. I haven't proceeded beyond this structural review, since this needs to change first.

kitallis · 2024-12-04T07:35:56Z

app/libs/coordinators/process_commit.rb

question: not sure I understand the point of this, why can't we reuse ProcessCommits? It's the exact same code with a small difference of: no "rest" commits and skipping immediate application.

The idea was to keep it seperate as the handled different scenarios. The file has since been discarded and ProcessCommits is being reused

kitallis · 2024-12-04T07:39:52Z

app/jobs/create_version_tag_job.rb

issue: this is entirely dead code.

kitallis · 2024-12-04T07:42:24Z

app/models/train.rb

+  before_create :set_release_branch, if: -> { branching_strategy == "trunk" }
+  before_create :set_build_queue_values, if: -> { branching_strategy == "trunk" }


Suggested change

before_create :set_release_branch, if: -> { branching_strategy == "trunk" }

before_create :set_build_queue_values, if: -> { branching_strategy == "trunk" }

before_create :set_release_branch, if: :trunk?

before_create :set_build_queue_values, if: :trunk?

kitallis · 2024-12-04T07:49:54Z

app/libs/triggers/trunk.rb

issue: this is entirely dead code.

kitallis · 2024-12-04T08:03:53Z

app/models/release_platform_run.rb

@@ -286,7 +286,7 @@ def update_last_commit!(commit)
  end

  def bump_version!
-    return unless version_bump_required?
+    return unless version_bump_required? || train.trunk?


Suggested change

return unless version_bump_required? || train.trunk?

return if train.trunk?

return unless version_bump_required?

Better to not chain conditionals with unless

kitallis · 2024-12-04T08:16:56Z

app/libs/coordinators/create_internal_release.rb

+    if release_platform_run.release.release_platform_runs.all? { |run| run.internal_releases.exists?(commit: commit) } &&
+        release_platform_run.train.trunk?
+
+      tag_name = "v#{release_platform_run.release_version}"
+      release_platform_run.train.create_tag!(tag_name, commit.commit_hash)
+
+      release_platform_run.release.release_platform_runs.each do |run|
+        run.update!(tag_name: tag_name)
+      end
+    end


issue: I think this creation & updating of tag should happen during the application of the commit, not here. This is about creating internal releases and its details. The tagging is about the commit, not about the builds.

kitallis · 2024-12-04T08:31:16Z

app/libs/coordinators/create_beta_release.rb

+    if release_platform_run.release.release_platform_runs.all? { |run| run.beta_releases.exists?(commit: commit) } &&
+        release_platform_run.train.trunk?
+
+      tag_name = "v#{release_platform_run.release_version}"
+      release_platform_run.train.create_tag!(tag_name, commit.commit_hash)
+
+      release_platform_run.release.release_platform_runs.each do |run|
+        run.update!(tag_name: tag_name)
+        ReleasePlatformRuns::TriggerWorkflowJob.perform_later(run.id, commit.id)
+      end
+    end


issue: the same issue applies here, as internal releases, but I'm also not certain what the need of triggering workflow job is here? The workflow run is normally triggered here anyway.

Could you help me understand the reasoning behind this?

The wrong approach was used when this was implemented. This has now been changed and it no longer handles the logic for creating and updating tags

gitstart-tramline · 2024-12-06T09:02:01Z

Thank you for the review. We will work on the changes and get back to you.

gitstart-tramline · 2024-12-23T07:15:51Z

Hi @kitallis, we have updated the PR to fix the functional and structural issues you mentioned above.
https://www.loom.com/share/380b57a1a5a943459cc0405c071c7bc0

Please review the PR. Thanks

gitstart-tramline · 2024-12-27T12:56:07Z

As soon as we start the release with Trunk, we should trigger the release for the HEAD of the working branch and not wait for a commit to show up before starting. The flow for when a commit lands and needs to be applied through the queue is correct, but should only happen for HEAD+N commits

The flow has been changes to achieve this.

The workflow runs are not being correctly found in trunk mode, they only seem to be found when the tag is used instead of the release branch. I tested this. The culprit is here. Perhaps we should only look for it via the commit SHA? We'll have to test this for Bitbucket and GitLab as well (I haven't done so yet).

This could not be achieved since the GitHub API requires the branch name or tag as mentioned here

We should disable all automations at the end. This flow breaks currently when you reach the end of the release. The FinalizeRelease should early return perhaps.

This was attended to.

We should not show this message in the finale phase and instead say something like the following in that card: Since we're doing a trunk-based branching strategy, there's no end-of-release housekeeping required on Tramline's end. We'll just refresh the DevOps reports and charts for you after the release is complete.

This was fixed

- store tag_name on commits, show tags for commits - move pre-release and post-release lpgic to trunk handlers - create post-release vcs release from last existing tag

Support proper trunk flow

222b06a

gitstart_bot added 2 commits November 20, 2024 15:46

Merge branch 'main' into TRAB-548

1911da9

Support proper trunk flow

5547d12

kitallis reviewed Dec 3, 2024

View reviewed changes

Merge branch 'main' into TRAB-548

be7c299

kitallis reviewed Dec 4, 2024

View reviewed changes

gitstart_bot added 2 commits December 23, 2024 14:07

Merge branch 'main' into TRAB-548

a45cfbd

requested changes

a319717

Merge branch 'main' into TRAB-548

550d90d

nid90 added 7 commits December 31, 2024 17:53

Fix fudge timestamp to handle non-string time

bced35a

Refactor trunk branching strategy flow

dd02ab7

- store tag_name on commits, show tags for commits - move pre-release and post-release lpgic to trunk handlers - create post-release vcs release from last existing tag

update brakeman

feda268

Merge branch 'main' into TRAB-548

149e51e

fix tag created stamp data

cf3530d

Merge branch 'main' into TRAB-548

5640974

Change end of release tasks for trunk

e57058e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support proper trunk flow #675

Support proper trunk flow #675

gitstart-app bot commented Nov 12, 2024 •

edited by nid90

Loading

gitstart-tramline commented Nov 12, 2024

gitstart-tramline commented Nov 20, 2024

kitallis left a comment •

edited

Loading

kitallis left a comment

kitallis Dec 4, 2024

gitstart-tramline Dec 27, 2024

kitallis Dec 4, 2024

kitallis Dec 4, 2024

kitallis Dec 4, 2024

kitallis Dec 4, 2024

kitallis Dec 4, 2024

kitallis Dec 4, 2024

gitstart-tramline Dec 27, 2024

gitstart-tramline commented Dec 6, 2024

gitstart-tramline commented Dec 23, 2024

gitstart-tramline commented Dec 27, 2024

		before_create :set_release_branch, if: -> { branching_strategy == "trunk" }
		before_create :set_build_queue_values, if: -> { branching_strategy == "trunk" }

	return unless version_bump_required? \|\| train.trunk?
	return if train.trunk?
	return unless version_bump_required?

Support proper trunk flow #675

Are you sure you want to change the base?

Support proper trunk flow #675

Conversation

gitstart-app bot commented Nov 12, 2024 • edited by nid90 Loading

Add trunk branching strategy

Loom

gitstart-tramline commented Nov 12, 2024

gitstart-tramline commented Nov 20, 2024

kitallis left a comment • edited Loading

Choose a reason for hiding this comment

kitallis left a 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

Choose a reason for hiding this comment

gitstart-tramline commented Dec 6, 2024

gitstart-tramline commented Dec 23, 2024

gitstart-tramline commented Dec 27, 2024

gitstart-app bot commented Nov 12, 2024 •

edited by nid90

Loading

kitallis left a comment •

edited

Loading