Skip to content
This repository has been archived by the owner on Dec 4, 2018. It is now read-only.

Handle failed CI cases better on subsequent runs #44

Merged
merged 2 commits into from
Feb 1, 2018

Conversation

aeijdenberg
Copy link
Contributor

Fixes some of the problems identified in #37.

This builds on top of #43.

We now have one actions code path that handles:

  1. No apps running.
  2. App running, no venerable.
  3. App not running, venerable app running.
  4. Both app and venerable app running.

Previously only (1) and (2) were handled correctly.

@aeijdenberg
Copy link
Contributor Author

(gentle ping)

We've been using this patch in prod now for a number of projects for a number of weeks, including health checkers that deploy a new release every 5 minutes, and we haven't seen issues.

I'll be happy to rebase to fix the minor merge conflict, but only if it's likely to get a review. :)

@drnic
Copy link

drnic commented Jan 31, 2018

@vito @contraband possible to merge; and then push into cf-resource?

Add additional dependencies so that we can tail app logs
Fix situations where CI has failed, and -venerable apps left dangling

May fix some of the edge cases identified in <contraband#37 (comment)>
@aeijdenberg
Copy link
Contributor Author

I've just rebased this on master, which now uses dep.

I spent a few hours banging my head against the wall, as dep ensure wouldn't report clean, even on master.

I eventually removed Gopkg.toml, Gopkg.lock and vendor/ dirs, and then ran dep init in a clean environment.

This seemed to create a cleaner set of Gopkg files, from which I used as a base to cherry-pick and merge both of the changes that we've been using the past few months:

  • Ability to tail logs
  • Fixes so that this won't fail if a previous deploy failed.

Hope this makes it easier to merge.

@aeijdenberg
Copy link
Contributor Author

Wow, this actually removes nearly 1 MILLION lines of code... (presumably because dep init now excludes vendored test packages by default)

@xoebus
Copy link
Contributor

xoebus commented Feb 1, 2018

Thank you! I'll review this tomorrow PST.

@xoebus xoebus merged commit aea62a3 into contraband:master Feb 1, 2018
@xoebus
Copy link
Contributor

xoebus commented Feb 1, 2018

Thank you!

Merged, released, and included in the cf-resource in concourse/concourse@40769f4.

@drnic
Copy link

drnic commented Feb 1, 2018 via email

@aeijdenberg
Copy link
Contributor Author

Thanks @xoebus - though note that I think you'll also need to merge cloudfoundry-community/cf-resource#40 in order to be able to pass the --show-app-log parameter through cf-resource.

@xoebus
Copy link
Contributor

xoebus commented Feb 1, 2018

Thanks for the heads up! I'll let @vito know.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants