Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reveal magic, parallel system-testing and system-testing simplification #1780

Merged
merged 3 commits into from
Nov 19, 2018

Conversation

cevich
Copy link
Member

@cevich cevich commented Nov 8, 2018

  • Previously, several magic strings were in place to affect cirrus-ci
    operations. Two were buried within scripts. One to optionally
    execute system-tests within a PR. Another to avoid re-building
    cache-images upon every merge.

    Move these magic strings out into the open, buy locating their
    logic up-front in the .cirrus.yml file. This improves
    readability and reduces surprise/astonishment at runtime.

  • Previously it was required to call the verify, unit, and integration
    scripts in order to build/install dependencies, and libpod. This
    wastes time during the (optional) system-testing, since the
    actual unit/integration testing is also happening in parallel.

    Consolidate only the distribution-specific build steps into the
    system-testing script. This way, only the required steps are performed
    in their respective (parallel) tasks.

@cevich cevich changed the title Reveal magic, parallel system-testing and system-testing simplification WIP: Reveal magic, parallel system-testing and system-testing simplification Nov 8, 2018
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2018
@cevich cevich force-pushed the un-magic branch 19 times, most recently from 0806628 to 1642549 Compare November 8, 2018 19:34
@cevich
Copy link
Member Author

cevich commented Nov 9, 2018

DO NOT MERGE: There is still a bug in cirrus, I am in contact with their engineering and working on a fix. They're typically really fast with these things, expect resolution today / early next week.

@rhatdan
Copy link
Member

rhatdan commented Nov 9, 2018

PASSES all test.
LGTM

@cevich
Copy link
Member Author

cevich commented Nov 9, 2018

@rhatdan thanks for taking a look. There are some not-so-obvious systemic bugs I need to work out. Hence the WIP and DO NOT MERGE. There's no better way to beat on the CI system than from w/in a PR, so there's a bunch of duct-tape in here temporarily. I'll strip all that out when the code is more solid (it's almost there).

@cevich cevich changed the title WIP: Reveal magic, parallel system-testing and system-testing simplification Reveal magic, parallel system-testing and system-testing simplification Nov 9, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2018
@cevich
Copy link
Member Author

cevich commented Nov 9, 2018

Update: Cirrus bug has been fixed, this PR is ready to go, PTAL. (Assuming happy tests)

@cevich
Copy link
Member Author

cevich commented Nov 13, 2018

Okay, this is working as designed: Having the magic words in the description caused this run to happen: https://cirrus-ci.com/task/5163575452631040 <-- Failed due to missing system tests (we know this).

Previously, several magic strings were in place to affect cirrus-ci
operations.  Two were buried within scripts.  One to optionally
execute system-tests within a PR. Another to avoid re-building
cache-images upon every merge.

Move these magic strings out into the open, buy locating their
logic up-front in the ``.cirrus.yml`` file.  This improves
readability and reduces surprise/astonishment at runtime.

Signed-off-by: Chris Evich <[email protected]>
Previously it was required to call the verify, unit, and integration
scripts in order to build/install dependencies, and libpod.  This
wastes time during the (optional) system-testing, since the
actual unit/integration testing is also happening in parallel.

Consolidate only the distribution-specific build steps into the
system-testing script.  This way, only the required steps are performed
in their respective (parallel) tasks.

Signed-off-by: Chris Evich <[email protected]>
***CIRRUS: REBUILD IMAGES***

Signed-off-by: Chris Evich <[email protected]>
@cevich
Copy link
Member Author

cevich commented Nov 13, 2018

Removed magic string, rebased and --force pushed: No system tests run 😄

@rhatdan
Copy link
Member

rhatdan commented Nov 17, 2018

/approve
LGTM
@edsantiago @baude @mheon @umohnani8 @vrothberg PTAL

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2018
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Nice work @cevich !

@edsantiago
Copy link
Member

LGTM too

@umohnani8
Copy link
Member

LGTM

@rhatdan
Copy link
Member

rhatdan commented Nov 19, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2018
@openshift-merge-robot openshift-merge-robot merged commit 47ffaae into containers:master Nov 19, 2018
@cevich cevich deleted the un-magic branch June 30, 2021 18:10
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants