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

fly: behaviour: add archive-pipeline command #5367

Closed
wants to merge 12 commits into from
Closed

Conversation

aoldershaw
Copy link
Contributor

@aoldershaw aoldershaw commented Mar 26, 2020

Existing Issue

branched of #5365 which should be merged first.
Fixes #5320 .

Changes proposed in this pull request

  • Add fly archive-pipeline
  • fly archive-pipeline supports a single pipeline (-p) or all pipelines in a team (--all)
  • fly archive-pipeline has a confirmation prompt before archiving any pipeline, overridable using -n

Contributor Checklist

Reviewer Checklist

  • Code reviewed
  • Tests reviewed
  • Documentation reviewed
  • Release notes reviewed
  • PR acceptance performed
  • New config flags added? Ensure that they are added to the BOSH and Helm packaging; otherwise, ignored for the integration tests (for example, if they are Garden configs that are not displayed in the --help text).

Jamie Klassen and others added 3 commits March 30, 2020 16:07
#5315

Driving this change with tests had some pain points. We decided to work
outside-in, starting with an ATC integration test. It was difficult to
move from the outer TDD loop to the inner - I was surprised by the
number of seemingly-far-flung changes that were required to move from
one failure to the next. These included:

* adding an entry to the auditor to overcome a panic
* modifying the `atc/api/present` package to link the DB entity to the
  API entity

both of these things feel so easy to forget, and it might be nice if
they could be described by a change in the same part of the codebase.

Signed-off-by: Jamie Klassen <cklassen@pivotal.io>
Co-authored-by: Bishoy Youssef <byoussef@pivotal.io>
Co-authored-by: Aidan Oldershaw <aoldershaw@pivotal.io>
#5315 - as per
#5346 (comment),
the ability to archive pipelines is now behind an api flag
`enable-archive-pipeline`.

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
Co-authored-by: James Thomson <jthomson@pivotal.io>
Not sure how that got in there!

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
YoussB and others added 9 commits April 6, 2020 15:38
Specifically for the pipelines/:pipeline_name/archive endpoint

Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>
Co-authored-by: Taylor Silva <tsilva@pivotal.io>
#5316

Rather than adding a concrete error type to check against, we elected to
create an interface for our domain-specific error to satisfy - see:

https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully

Following the example of trigger-job we decided that 409 Conflict was the
most logical http status to use to represent this kind of policy
violation.

We decided to add the `Conflict` interface to the `db` package because
it seems there is no more-appropriate "business logic" or "policy" layer
to place this kind of validation error. Since multiple backend members
in the web node interact with pipelines directly via the database, it
seems justified to place the business logic as close to that integration
boundary as possible.

Recent painful experiences with observability motivated the choice to
treat logging more like a feature, and have unit tests that specifically
require it.

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
Co-authored-by: Jamie Klassen <cklassen@pivotal.io>
Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
Co-authored-by: Jamie Klassen <cklassen@pivotal.io>
#5318

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
#5319

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
Co-authored-by: James Thomson <jthomson@pivotal.io>
#5319

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
Co-authored-by: James Thomson <jthomson@pivotal.io>
#5319

Now that we have an `ArchivePipeline` method in go-concourse, we can use
that in the integration tests for archiving pipelines (rather than
caling the endpoint directly).

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
Co-authored-by: James Thomson <jthomson@pivotal.io>
#5320

Adds an archive-pipeline command that functions nearly identically to
pause-pipeline

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
Co-authored-by: James Thomson <jthomson@pivotal.io>
#5320

Since archiving pipelines can be destructive, the fly archive-pipelines
command should have a confirm dialog before making any changes. This is
overridable by a --non-interactive flag.

In the case of --all, print a table of all the pipelines that will be
archived if the user confirms.

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
Co-authored-by: James Thomson <jthomson@pivotal.io>
@YoussB
Copy link
Member

YoussB commented Apr 7, 2020

closing this pr as its commits are covered in #5387.

@YoussB YoussB closed this Apr 7, 2020
Epic #5055 automation moved this from To do to Done Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Epic #5055
  
Done
Development

Successfully merging this pull request may close these issues.

Implement fly archive-pipeline
3 participants