Old, modified way of deploying repository #205

rzadp · 2019-09-11T08:47:32Z

This PR reverts the changes done in New way of deploying repository #195 , with modification to make it work for forked PRs.
Closes Investigate repository deployment method #200
Closes remove dependency on rsync #202
Note that this is a forked PR

rzadp · 2019-09-13T11:32:19Z

deployment-e2e/molecule/repo/converge.yml

@@ -3,6 +3,9 @@
  hosts: all
  become: true
  tasks:
+    - import_role:
+        name: ../../../deployment/roles/common
+        tasks_from: dependencies


Need to add dependencies in this test scenario because otherwise there is no git installed on host machine

rzadp · 2019-09-13T11:40:45Z

deployment/roles/common/tasks/repo.yml

-    tar zxfv monorepo.tar.gz && rm monorepo.tar.gz
-    mkdir -p contracts && tar zxfv contracts.tar.gz -C ./contracts && rm contracts.tar.gz
+    # If this is a forked PR, the branch needs to be fetched in a special way
+    [ ! "CIRCLE_PR_NUMBER" = "" ] && git fetch --force origin -- "{{ bridge_repo_branch }}/head:remotes/origin/{{ bridge_repo_branch }}"


CIRCLE_PR_NUMBER is only available on forked PR. So if it is available, we fetch the forked branch in a special way.

akolotov · 2019-09-13T16:10:40Z

deployment-e2e/molecule.sh

@@ -2,8 +2,10 @@
 cd $(dirname $0)
 set -e # exit when any command fails

+CODEBASE_BRANCH=${CIRCLE_BRANCH-$(git symbolic-ref --short HEAD)}


will it still be possible to run e2e deployment tests locally?

Good question.

In a normal repository - yes

In a forked repository - I will work on it now and make sure that it is possible

akolotov · 2019-09-13T16:19:51Z

deployment/roles/common/tasks/repo.yml

-    tar zxfv monorepo.tar.gz && rm monorepo.tar.gz
-    mkdir -p contracts && tar zxfv contracts.tar.gz -C ./contracts && rm contracts.tar.gz
+    # If this is a forked PR, the branch needs to be fetched in a special way
+    [ ! "CIRCLE_PR_NUMBER" = "" ] && git fetch --force origin -- "{{ bridge_repo_branch }}/head:remotes/origin/{{ bridge_repo_branch }}"


does it make sense to limit the number of downloaded artifacts by --depth parameter?

Good idea. Done in a4caf18

rzadp · 2019-09-16T15:11:50Z

@akolotov I have trouble making it work on forked PRs. Turns out, the tests were passing before for the wrong reason. I found that I had two branches named the same way on both forked and original repository, and it was actually checking out the wrong one!

My current opinion is that approach to git clone is arguably more complex than the approach we have currently with tar archives.

When running deployment-e2e tests, and also when manually testing deployment on DigitalOcean/AWS, the approach to git clone needs to take into account 4 cases:

Locally on original repository
- The local branch needs to be pushed to remote (in order to be git cloned later)
- Need to make sure that in fact given branch is checked out and tested, and not the default master
CI testing PR from original repository
- Need to make sure that in fact given branch is checked out and tested
Locally on forked repository
- The local branch needs to be pushed to remote
- Need to make sure that in fact given branch is checked out and tested
- Need to make sure that in fact the forked repository is tested, and not the default poanetwork/tokenbridge
CI testing PR from forked repository
- Need to make sure that in fact given branch from forked repository is checked out and tested
- This branch must be fetched and checked-out in a special way

If something gets wrong, it is easy to overlook, because the codebase from original repository's master has high chance to just work. Just like I overlooked it with two branches.

I think that our current approach is better and simpler. The tasks to tar/untar are complex, but the principles are simple:

Locally, the code you see is deployed / tested. No matter if you have original repository, forked repository, or if you downloaded repository .zip
When testing original/forked PR, CI checks out proper code. This code gets tested.

In conclusion, I argue that it would be better to leave our current approach with copying repository as it is.
@akolotov What do you think?

akolotov · 2019-09-17T10:31:53Z

I think that our current approach is better and simpler. The tasks to tar/untar are complex, but the principles are simple:

Locally, the code you see is deployed / tested. No matter if you have original repository, forked repository, or if you downloaded repository .zip

When testing original/forked PR, CI checks out proper code. This code gets tested.

In conclusion, I argue that it would be better to leave our current approach with copying repository as it is.
@akolotov What do you think?

Thanks @rzadp for detailed explanation. Please add this comment to the issue #200.

akolotov · 2019-09-17T10:33:35Z

If nothing is going to be changed as part of this PR, please close it.

rzadp force-pushed the forked-pr branch from 4190f5c to e888801 Compare September 13, 2019 08:46

rzadp commented Sep 13, 2019

View reviewed changes

rzadp force-pushed the forked-pr branch from 61f8956 to c6a2bed Compare September 13, 2019 12:32

rzadp added 10 commits September 13, 2019 16:41

Forked PR

290c742

vars

1d02ff2

CIRCLE_BRANCH

e31d0ce

Bring back old way of deploying

f7419ef

All jobs

06a8a19

Refspec

83e34df

Remove rsync dependency

b44b6ac

Repo dependencies

125dbc7

Fetch forked branch

e8e24d7

Remove .git check

fe4de20

rzadp force-pushed the forked-pr branch from c6a2bed to fe4de20 Compare September 13, 2019 14:42

rzadp changed the title ~~[WIP] Forked PR~~ Old, modified way of deploying repository Sep 13, 2019

rzadp marked this pull request as ready for review September 13, 2019 15:02

rzadp requested review from akolotov, ArseniiPetrovich and phahulin September 13, 2019 15:02

akolotov reviewed Sep 13, 2019

View reviewed changes

Depth of 1

a4caf18

rzadp closed this Sep 16, 2019

rzadp deleted the forked-pr branch September 16, 2019 11:37

rzadp restored the forked-pr branch September 16, 2019 11:37

rzadp added 2 commits September 16, 2019 13:52

Merge master into forked-pr

357f606

CODEBASE_REPO for running deployment-e2e locally on forked repositories

fd84d61

rzadp reopened this Sep 16, 2019

Pass CIRCLE_PR_NUMBER

77b02c5

rzadp force-pushed the forked-pr branch from dc04bb4 to 1b3fa6c Compare September 16, 2019 14:19

Corrected checkout

16e8539

rzadp force-pushed the forked-pr branch from 1b3fa6c to 16e8539 Compare September 16, 2019 14:26

CIRCLE_PR_NUMBER: $CIRCLE_PR_NUMBER

71721c7

rzadp mentioned this pull request Sep 17, 2019

Investigate repository deployment method #200

Closed

rzadp closed this Sep 17, 2019

rzadp deleted the forked-pr branch September 20, 2020 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Old, modified way of deploying repository #205

Old, modified way of deploying repository #205

rzadp commented Sep 11, 2019 •

edited

Loading

rzadp Sep 13, 2019

rzadp Sep 13, 2019

akolotov Sep 13, 2019

rzadp Sep 16, 2019

akolotov Sep 13, 2019

rzadp Sep 16, 2019

rzadp commented Sep 16, 2019

akolotov commented Sep 17, 2019 •

edited

Loading

akolotov commented Sep 17, 2019

Old, modified way of deploying repository #205

Old, modified way of deploying repository #205

Conversation

rzadp commented Sep 11, 2019 • edited Loading

rzadp Sep 13, 2019

Choose a reason for hiding this comment

rzadp Sep 13, 2019

Choose a reason for hiding this comment

akolotov Sep 13, 2019

Choose a reason for hiding this comment

rzadp Sep 16, 2019

Choose a reason for hiding this comment

akolotov Sep 13, 2019

Choose a reason for hiding this comment

rzadp Sep 16, 2019

Choose a reason for hiding this comment

rzadp commented Sep 16, 2019

akolotov commented Sep 17, 2019 • edited Loading

akolotov commented Sep 17, 2019

rzadp commented Sep 11, 2019 •

edited

Loading

akolotov commented Sep 17, 2019 •

edited

Loading