Persist source sha and target sha for merge pipelines

added 1 commit

99ae543e - Persist source sha and target sha for merge pipelines

marked the checklist item Changelog entry added, if necessary as completed

marked the checklist item Documentation created/updated via this MR as completed

changed the description

changed milestone to %11.9

added Category:Continuous Delivery Category:Continuous Integration Deliverable DogfoodingRebuild in GitLab GitLab Premium PM Ready Product Vision 2018 backend customer direction frontend internal customer + 1 deleted label

assigned to @dosuken123

added 1 commit

f74995ba - Fix doc

Compare with previous version

	3 Warnings
	This merge request is missing the database label.
	This merge request is missing the ~Documentation label.
	314062fe: This commit’s subject line is acceptable, but please try to reduce it to 50 characters.

	2 Messages
	This merge request adds or changes files that require a review from the Database team.
	This merge request adds or changes files that require a review from the Technical Writing team whether before or after merging.

Database Review

The following files require a review from the Database team:

db/migrate/20190220150130_add_extra_shas_to_ci_pipelines.rb
db/schema.rb

To make sure these changes are reviewed, take the following steps:

Edit your merge request, and add gl-database to the list of Group approvers.
Mention @gl-database in a separate comment, and explain what needs to be reviewed by the team. Please don't mention the team until your changes are ready for review.

Documentation review

The following files will require a review from the Technical Writing team:

doc/ci/variables/README.md

Per the documentation workflows, the review may occur prior to merging this MR or after a maintainer has agreed to review and merge this MR without a tech writer review. (Meanwhile, you are welcome to involve a technical writer at any time if you have questions about writing or updating the documentation. GitLabbers are also welcome to use the #docs channel on Slack.)

The technical writer who, by default, will review this content or collaborate as needed is dependent on the DevOps stage to which the content applies:

Tech writer	Stage(s)
`@marcia`	~Create ~Release + development guidelines
`@axil`	~Distribution Gitaly ~Gitter ~Monitor ~Package ~Secure
`@eread`	~Manage ~Configure ~Geo ~Verify
`@mikelewis`	~Plan

To request a review prior to merging

Assign the MR to the applicable technical writer, above. If you are not sure which category the change falls within, or the change is not part of one of these categories, mention one of the usernames above.

To request a review to commence after merging

Create an issue for a doc review using the Doc Review template and assign it to the applicable technicial writer from the table.

Reviewer roulette

Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.

Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.

Category	Reviewer	Maintainer
backend	Jan Provaznik (`@jprovaznik`)	Robert Speicher (`@rspeicher`)
database		Andreas Brandl (`@abrandl`)
~Documentation		Mike Lewis (`@mikelewis`)

Generated by Danger

marked the checklist item Tests added for this feature/bug as completed

added 1 commit

be1c53c8 - Persist source sha and target sha for merge pipelines

Compare with previous version

@ayufan Would you mind doing an early review?

assigned to @ayufan

changed the description

added 1 commit

8c5c39e9 - Persist source sha and target sha for merge pipelines

Compare with previous version

@dosuken123

I think that this is good direction, but:

I think we should pass source_sha and target_sha as params, instead of reading that from merge request,
If we want to read that during call, it should rather be filled by reading the commit.id and filling the source_sha and target_sha from commit parents, as this allows us to efficiently store commit parents where commit is identified by ci_pipelines.sha,
I don't think that we should store BLANK_SHA, it does not make sense. We should rather keep it nil,
I have comments to your snippet, as for merge pipeline you keep using as ref source branch, maybe we should rather use ref that actually reflects used merge ref?

I don't know what are the consequences of using that ref, but I assume that this is what we should be doing anyway. It will definitely break detached pipeline today for old runners, so maybe we could only require /merge to be used like that and transition to use /head with %12.0?

## How to create a detached pipeline?
CreatePipelineService.new(project, user, ref: "refs/merge-requests/iid/head", checkout_sha: merge_request.source_branch_sha)
                     .execute(:merge_request, merge_request: merge_request)

## How to create a merge pipeline?
CreatePipelineService.new(project, user, ref: "refs/merge-requests/iid/merge", checkout_sha: merge_sha, source_sha: merge_request.diff_head_sha, target_sha: merge_request.target_branch_sha)
                     .execute(:merge_request, merge_request: merge_request)

## How to evaluate if it's a detached pipeline?
pipeline.merge_request? && pipeline.target_sha.nil?

## How to evaluate if it's a merge pipeline?
pipeline.merge_request? && pipeline.target_sha.present?

## How to evaluate if the merge pipeline uses HEAD of target branch (If false, don't merge)?
pipeline.merge_request? && pipeline.target_sha == merge_request.target_branch_sha

assigned to @dosuken123

added 1 commit

dce1b794 - Use binary column

Compare with previous version

added 1 commit

bfac467f - Allow nil source sha and target sha

Compare with previous version

added 1 commit

37a2f3ee - Allow nil source sha and target sha

Compare with previous version

resolved all discussions

added 1 commit

0b01b5e7 - Fix doc

Compare with previous version

added 161 commits

0b01b5e7...9a4ef1e3 - 160 commits from branch master
ac843293 - Persist source sha and target sha for merge pipelines

Compare with previous version

changed the description

added 1 commit

79de30bb - Add scopes

Compare with previous version

@ayufan

I don't know what are the consequences of using that ref, but I assume that this is what we should be doing anyway. It will definitely break detached pipeline today for old runners, so maybe we could only require /merge to be used like that and transition to use /head with %12.0?

We'll try to implement this, however, we have to measure the impact on the potential refactoring at first. Also, storing full ref path is out of scope in this MR. This MR is simply focusing on allowing the system to store target_sha and source_sha in order for the detection of detached/merge MR pipelines.

added 1 commit

b5fcfd8c - Persist source sha and target sha for merge pipelines

Compare with previous version

@reprazent Would you mind taking a look at this? Thanks! We're planning to merge this today!

assigned to @reprazent

@dosuken123 Only nitpicks from my side.

assigned to @dosuken123

resolved all discussions

added 1 commit

433aeb88 - Persist source sha and target sha for merge pipelines

Compare with previous version

@reprazent Thank you for reviewing!

@ayufan Would you mind taking a look at this MR?

assigned to @ayufan

changed the description

added 1 commit

1e1c8cf2 - Persist source sha and target sha for merge pipelines

Compare with previous version

added 1 commit

e891be3e - Persist source sha and target sha for merge pipelines

Compare with previous version

@gl-database Hi DB specialists I'd like to ask you to review for database related changes.

db/migrate/20190220150130_add_extra_shas_to_ci_pipelines.rb
db/schema.rb

It's simply adding two columns source_sha and target_sha to ci_pipelines table. So that I don't think there is a problematic code. Please let me know if you have any questions.

We can compare bytea type column and varchar type column on PostgreSQL by using encode and decode.

Here is a brief test on PostgreSQL instance. Both encoding the value to varchar and decoding the value to bytea correctly match the row.

gitlabhq_development_ce=# select * from gpg_signatures where commit_sha = '6ac2afa3b2278cbf147a717894eb7643615d0884';
 id | created_at | updated_at | project_id | gpg_key_id | commit_sha | gpg_key_primary_keyid | gpg_key_user_name | gpg_key_user_email | verification_status | gpg_key_subkey_id
----+------------+------------+------------+------------+------------+-----------------------+-------------------+--------------------+---------------------+-------------------
(0 rows)

gitlabhq_development_ce=# select * from gpg_signatures where encode(commit_sha, 'hex') = '6ac2afa3b2278cbf147a717894eb7643615d0884';
 id |          created_at           |          updated_at           | project_id | gpg_key_id |                 commit_sha                 | gpg_key_primary_keyid | gpg_key_user_name | gpg_key_user_email | verification_status | gpg_key_subkey_id
----+-------------------------------+-------------------------------+------------+------------+--------------------------------------------+-----------------------+-------------------+--------------------+---------------------+-------------------
  1 | 2018-12-06 14:25:52.860447+09 | 2018-12-06 14:25:52.860447+09 |         50 |            | \x6ac2afa3b2278cbf147a717894eb7643615d0884 | \x7d7005a5732d1234    |                   |                    |                   5 |
(1 row)

gitlabhq_development_ce=# select * from gpg_signatures where commit_sha = decode('6ac2afa3b2278cbf147a717894eb7643615d0884', 'hex');
 id |          created_at           |          updated_at           | project_id | gpg_key_id |                 commit_sha                 | gpg_key_primary_keyid | gpg_key_user_name | gpg_key_user_email | verification_status | gpg_key_subkey_id
----+-------------------------------+-------------------------------+------------+------------+--------------------------------------------+-----------------------+-------------------+--------------------+---------------------+-------------------
  1 | 2018-12-06 14:25:52.860447+09 | 2018-12-06 14:25:52.860447+09 |         50 |            | \x6ac2afa3b2278cbf147a717894eb7643615d0884 | \x7d7005a5732d1234    |                   |                    |                   5 |
(1 row)