Checkout matching origin/openshift-ansible branch when syncing repos #449

jhadvig · 2017-07-21T13:28:51Z

@stevekuznetsov here is the fix for oct sync ... which should checkout proper _TARGET_BRANCH
So in case of PR to origin, OCT will checkout openshift-ansible in matching version and vice versa.
response to #434 (comment)

Fixes openshift/origin#15186

jhadvig · 2017-07-21T13:30:30Z

@Kargakis fyi here is the fix for the issue that blocking 1.5 merge queue...

0xmichalis · 2017-07-21T13:32:43Z

You mean the real fix to your workaround, right? Unfortunately there are two more issues that block release-1.5 merges :(

0xmichalis · 2017-07-21T13:36:25Z

Unfortunately there are two more issues that block release-1.5 merges :(

http://post-office.corp.redhat.com/archives/aos-devel/2017-July/msg00736.html

stevekuznetsov · 2017-07-21T13:39:56Z

sjb/generate.py

    for repository in job_config.get("sync_repos", []):
        if repository.get("type", None) == "pull_request":
            sync_actions.append(PullRequestSyncAction(repository["name"]))
        else:
-            sync_actions.append(SyncAction(repository["name"]))
+            # if test is a PR, point to dependency repository to the PR's repository branch
+            repository_target_branch = get_dependency_repo(repository["name"]) if is_pull_request else repository["name"]


Resolve the correct branch here, don't pass info down into the actions.

I'm resolving it here... we dont know the branch name until the test is triggered. Upon the generation process we dont know whats the branch name, we have only info about what repos are synced and upon that we can assume what the _TARGET_BRANCH should be.
I've removed the additional action and to keep it simple since the get_dependency_repo will determine the right repository_target_branch ... also not really sure that it makes sense to check for the branch name in the _SYNC_ACTION_TEMPLATE since there should be always a matching version between origin and o-a

stevekuznetsov · 2017-07-24T12:52:54Z

sjb/actions/repo_sync.py

-_SYNC_ACTION_TEMPLATE = Template("""oct sync remote {{ repository }} --branch "${{ '{' }}{{ repository | replace('-', '_') | upper }}_TARGET_BRANCH}" """)
-
-_SYNC_DESCRIPTION_TEMPLATE = Template("""Using the <a href="https://github.com/openshift/{{ repository }}/tree/${{ '{' }}{{ repository | replace('-', '_') | upper }}_TARGET_BRANCH}">{{ repository }} ${{ '{' }}{{ repository | replace('-', '_') | upper }}_TARGET_BRANCH}</a> branch.""")
+_SYNC_ACTION_TEMPLATE = Template("""oct sync remote {{ repository }} --branch "${{ '{' }}{{ repository_target_branch | replace('-', '_') | upper }}_TARGET_BRANCH}" """)


repository_target_branch is used to create a ${X_TARGET_BRANCH} var? So it's not a branch, but a repo? This is misleading name -- should have a comment as well in the class doc

yeah thats what the var is for.. sorry my brain was hurting when I was writing it 🤕 ... will update the name and class docs

stevekuznetsov · 2017-07-24T14:32:16Z

sjb/actions/repo_sync.py

-
-_SYNC_DESCRIPTION_TEMPLATE = Template("""Using the <a href="https://github.com/openshift/{{ repository }}/tree/${{ '{' }}{{ repository | replace('-', '_') | upper }}_TARGET_BRANCH}">{{ repository }} ${{ '{' }}{{ repository | replace('-', '_') | upper }}_TARGET_BRANCH}</a> branch.""")
+_SYNC_ACTION_TEMPLATE = Template("""oct sync remote {{ repository }} --branch "${{ '{' }}{{ dependency_repository | replace('-', '_') | upper }}_TARGET_BRANCH}" """)
+_SYNC_DESCRIPTION_TEMPLATE = Template("""Using the <a href="https://github.com/openshift/{{ repository }}/tree/${{ '{' }}{{ dependency_repository | replace('-', '_') | upper }}_TARGET_BRANCH}">{{ repository }} ${{ '{' }}{{ dependency_repository | replace('-', '_') | upper }}_TARGET_BRANCH}</a> branch.""")


If there is no diff here can we remove the diff please

stevekuznetsov · 2017-07-24T14:32:35Z

sjb/actions/pull_request_sync.py

@@ -77,7 +77,7 @@ class PullRequestSyncAction(Action):

    def __init__(self, repository):
        self.repository = repository
-        self.parent_sync = SyncAction(repository)
+        self.parent_sync = SyncAction(repository, repository)


Can we use dependency_repository=repository to explain what this is here please

stevekuznetsov · 2017-07-24T14:32:53Z

sjb/actions/repo_sync.py

-            title=_SYNC_TITLE_TEMPLATE.render(repository=self.repository),
-            command=_SYNC_ACTION_TEMPLATE.render(repository=self.repository)
-        )]
+        return [render_task(title=_SYNC_TITLE_TEMPLATE.render(repository=self.repository),command=_SYNC_ACTION_TEMPLATE.render(repository=self.repository, dependency_repository=self.dependency_repository))]


Don't change the formatting here

stevekuznetsov · 2017-07-24T14:33:49Z

sjb/generate.py

+
+    # Determine the name of dependency repository that will serve as prefix to 'XXX_TARGET_BRANCH'
+    # variable, so we can set the matching branch for input repository.
+    # So far the repository that are dependent to each other are 'origin' and 'openshift-ansible'.


Comment is out of date already -- remove it, rest of code is easy enough to follow

stevekuznetsov · 2017-07-24T14:40:06Z

sjb/generate.py

+    # variable, so we can set the matching branch for input repository.
+    # So far the repository that are dependent to each other are 'origin' and 'openshift-ansible'.
+    # Otherwise set the dependency repository to input repository name.
+    def get_dependency_repo(repository_name):


Maybe we could use:

repo_dependencies = { "origin": ["openshift-ansible"], "origin-aggregated-logging": ["openshift-ansible"], "openshift-ansible": ["origin", "openshift-ansible"] } def get_parent_repo(dependent_repo_name): for parent in repo_dependencies[dependent_repo_name]: if parent in repo_names: return parent return dependent_repo_name

jhadvig · 2017-07-24T20:17:24Z

@stevekuznetsov comments addressed .. PTAL

stevekuznetsov · 2017-07-24T21:14:47Z

sjb/generate.py

@@ -99,11 +99,38 @@ def load_configuration(config_path):

    # next, repositories will be synced to the remote VM
    sync_actions = []
+    repo_names=[repository["name"] for repository in job_config.get("sync_repos", [])]
+
+    # determine if test is a pull request


This does not matter -- if we are running branch builds and build a release branch of repo x we still want the correct branch of other repos as well.

jhadvig · 2017-07-25T10:10:28Z

@stevekuznetsov comments addressed. PTAL

stevekuznetsov · 2017-07-25T13:40:27Z

sjb/generated/test_branch_openshift_ansible_logging.xml

 &lt;/div&gt;</description>
    </hudson.plugins.descriptionsetter.DescriptionSetterBuilder>
        <hudson.tasks.Shell>
          <command>#!/bin/bash
 SCRIPT_START_TIME=&#34;$( date +%s )&#34; &amp;&amp; export SCRIPT_START_TIME &amp;&amp; echo &#34;########## STARTING STAGE: SYNC ORIGIN-AGGREGATED-LOGGING REPOSITORY ##########&#34; &amp;&amp; trap &#39;export status=FAILURE&#39; ERR &amp;&amp; trap &#39;set +o xtrace; SCRIPT_END_TIME=&#34;$( date +%s )&#34;; ELAPSED_TIME=&#34;$(( SCRIPT_END_TIME - SCRIPT_START_TIME ))&#34;; echo &#34;########## FINISHED STAGE: ${status:-SUCCESS}: SYNC ORIGIN-AGGREGATED-LOGGING REPOSITORY [$( printf &#34;%02dh %02dm %02ds&#34; &#34;$(( ELAPSED_TIME/3600 ))&#34; &#34;$(( (ELAPSED_TIME%3600)/60 ))&#34; &#34;$(( ELAPSED_TIME%60 ))&#34; )] ##########&#34;&#39; EXIT &amp;&amp; set -o errexit -o nounset -o pipefail -o xtrace &amp;&amp; if [[ -s &#34;${WORKSPACE}/activate&#34; ]]; then source &#34;${WORKSPACE}/activate&#34;; fi
-oct sync remote origin-aggregated-logging --branch &#34;${ORIGIN_AGGREGATED_LOGGING_TARGET_BRANCH}&#34; </command>
+oct sync remote origin-aggregated-logging --branch &#34;${OPENSHIFT_ANSIBLE_TARGET_BRANCH}&#34; </command>


This job now just switched the branches -- logging uses ansible, and vice versa. This is not what we wanted, is it?

well this is the branch job that I was testing for with the is_pull_request variable... I thought that since you told me that it shouldnt matter that we want to checkout the branches the same way as we did with the PR tets

The logic -- whether it's for a PR or for not -- is wrong, though. We need the branches to converge on one variable, right? Not have them all switch.

No the PR logic should be fine.
eg: test_pull_request_openshift_ansible_extended_conformance_install
PR is sent against branch of o-a, so we have to checkout the same branch for origin and thats what the logis is/was (with the is_pull_request) doing. So in this case we set the dependency repository to the same branch as the repository's branch that the PR is sent against.

But for the 'branch' TCs I'm not sure how to set the the correct branch since we dont have clue whats the main tested repo and which is the dependency...
For that reason I would rather keep it simple and just cover the PR TCs.

Thoughts ?

I think your approach is valid -- sorry for nixing the PR-only before, it technically should work for the branch jobs too but as you note it is very hard to know which branch should win out.

stevekuznetsov suggested changes Jul 21, 2017

View reviewed changes

stevekuznetsov suggested changes Jul 24, 2017

View reviewed changes

jhadvig force-pushed the build_target2 branch 2 times, most recently from a8383f0 to cead4a1 Compare July 24, 2017 14:04

stevekuznetsov reviewed Jul 24, 2017

View reviewed changes

jhadvig force-pushed the build_target2 branch from cead4a1 to c5ff17e Compare July 24, 2017 20:16

stevekuznetsov approved these changes Jul 24, 2017

View reviewed changes

stevekuznetsov suggested changes Jul 24, 2017

View reviewed changes

jhadvig force-pushed the build_target2 branch from c5ff17e to dd516d3 Compare July 25, 2017 10:10

stevekuznetsov reviewed Jul 25, 2017

View reviewed changes

Checkout matching origin/openshift-ansible branch when syncing repos

bd63f1b

jhadvig force-pushed the build_target2 branch from dd516d3 to bd63f1b Compare July 27, 2017 13:45

stevekuznetsov approved these changes Jul 27, 2017

View reviewed changes

stevekuznetsov merged commit 5b73212 into openshift-eng:master Jul 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checkout matching origin/openshift-ansible branch when syncing repos #449

Checkout matching origin/openshift-ansible branch when syncing repos #449

jhadvig commented Jul 21, 2017 •

edited

Loading

jhadvig commented Jul 21, 2017

0xmichalis commented Jul 21, 2017

0xmichalis commented Jul 21, 2017

stevekuznetsov Jul 21, 2017

jhadvig Jul 24, 2017

stevekuznetsov Jul 24, 2017

jhadvig Jul 24, 2017

stevekuznetsov Jul 24, 2017

stevekuznetsov Jul 24, 2017

stevekuznetsov Jul 24, 2017

stevekuznetsov Jul 24, 2017

stevekuznetsov Jul 24, 2017

jhadvig commented Jul 24, 2017

stevekuznetsov Jul 24, 2017

jhadvig commented Jul 25, 2017

stevekuznetsov Jul 25, 2017

jhadvig Jul 25, 2017 •

edited

Loading

stevekuznetsov Jul 25, 2017

jhadvig Jul 27, 2017

stevekuznetsov Jul 27, 2017

Checkout matching origin/openshift-ansible branch when syncing repos #449

Checkout matching origin/openshift-ansible branch when syncing repos #449

Conversation

jhadvig commented Jul 21, 2017 • edited Loading

jhadvig commented Jul 21, 2017

0xmichalis commented Jul 21, 2017

0xmichalis commented Jul 21, 2017

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

jhadvig commented Jul 24, 2017

Choose a reason for hiding this comment

jhadvig commented Jul 25, 2017

Choose a reason for hiding this comment

jhadvig Jul 25, 2017 • edited Loading

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

jhadvig commented Jul 21, 2017 •

edited

Loading

jhadvig Jul 25, 2017 •

edited

Loading