atc: behaviour: add ArchivePipeline endpoint

[jamieklassen]

Existing Issue

Based on #5329, which should be merged first.
Fixes #5315.

Changes proposed in this pull request

• Adds an archived column to the pipelines table
• the ArchivePipeline endpoint strictly updates this field to true
• access is limited to team owners.

Contributor Checklist

• Unit tests
• Integration tests (if applicable)
• Updated documentation (located at https://github.com/concourse/docs)
• Updated release notes (located at https://github.com/concourse/concourse/tree/master/release-notes)

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).

note - we will have to add the flags to bosh and helm, but that won't be part of this PR.

[taylorsilva]

@jomsie and I did acceptance on this and it does what it says it does!

We docker-compose up -d --build and set a pipeline. We then connected to the db:

$ psql -h localhost -p 6543 -U dev -W -d concourse

and checked the pipelines table immediately after setting the pipeline:

$ select name,archived from pipelines;
 name  | archived
-------+----------
 test  | f
(1 rows)

then we archived the pipeline by running fly -t local curl api/v1/teams/main/pipelines/test2/archive "" -- -X PUT and checked the db again:

$ select name,archived from pipelines;
 name  | archived
-------+----------
 test  | t
(1 rows)

It did what it says it would do! yay!

One little nit about one of the tests, otherwise this seems good to go!

[taylorsilva]

Maybe I'm missing something but it looks like this is set in the BeforeEach already

Suggested change

atcURL := fmt.Sprintf("http://localhost:%v", cmd.BindPort)

[jamieklassen]

hahaha @aoldershaw he caught us. we noticed this in a later PR

[jamieklassen]

@taylorsilva @jomsie nice thorough acceptance. Since this depends on #5329 I think it would be enough to check the results of fly pipelines --json or fly curl /api/v1/pipelines to witness that the archived field is appropriately updated. Getting ahead of designs like concourse/rfcs#47, can we actually loop back (rebasing #5351, #5359, #5365 and #5367 appropriately) and hide this API endpoint behind a feature flag? Obviously we have no reason to expect it to be expensive, but the subsequent stories in this epic give it the potential to be somewhat disruptive in terms of hiding pipelines. I can imagine a few especially-conservative operators being very careful rolling out the feature.

EDIT: I can't request changes on my own PR.

[cirocosta]

loooking goood, added some comments. great job!

[cirocosta]

In the same suite there are quite a bit of tests that also validate what happens when an unauthorized users tries to reach the corresponding endpoint

 ~/workspace/concourse/atc/api $ ag "401" ./pipelines_test.go
415:                    It("returns 401", func() {
509:                            It("returns 401", func() {
589:                                    It("returns 401", func() {
867:                    It("returns 401 Unauthorized", func() {
946:                    It("returns 401", func() {
1066:                   It("returns 401 Unauthorized", func() {
1146:                   It("returns 401 Unauthorized", func() {
1224:                   It("returns 401 Unauthorized", func() {
1335:                   It("returns 401 Unauthorized", func() {
1538:                   It("returns 401 Unauthorized", func() {
1616:                   It("returns 401 Unauthorized", func() {
1644:                           It("returns 401", func() {
1830:                   It("returns 401", func() {

e.g.

concourse/atc/api/pipelines_test.go

Lines 409 to 417 in 004e924

	Context("when not authenticated", func() {
	BeforeEach(func() {
	fakeaccess.IsAuthenticatedReturns(false)
	})
	It("returns 401", func() {
	Expect(response.StatusCode).To(Equal(http.StatusUnauthorized))
	})
	})

wdyt of adding tests for this one too? it seems to me that the big purpose of this more "end-to-end" approach of testing at the api level would be to catch these things 🤔 please correct me if i'm wrong! 😁

[jamieklassen]

hmm, we tried to drive out this feature by pretty rigorously practicing TDD. While pairing with @aoldershaw and @YoussB on different occasions, I recall weighing the decision to add these kinds of tests, and they felt redundant since we were already adding the table test entries for accessor itself. Indeed I think if we followed the boilerplate of the other tests, the newly-added tests would never actually go red. However, leaving them out is really only safe if we trust the other components we are integrating with and we have a solid contract with them. For example, I think there is a component with a name like 'api auth wrappa' that is conceptually 'responsible' for the behaviours you are describing, but we don't have a unit test anywhere saying "this code delegates to the api auth wrappa", and maybe we should? After all, if we ended up switching away from using the api auth wrappa, there would be nothing ensuring this "sensible default" behaviour.

[cirocosta]

oh was this intentionally left here? if one wants to get a sense of "this endpoint was hit", one could leverage the audit logs

[jamieklassen]

Given that I find myself reading other people's logs (more on that below), I feel motivated to make the system more verbose by default, just in case. This line is actually enforced by a unit test, because I don't want to rely on other people to turn on auditing if they're then going to be needing my advice on the state of their cluster.

[cirocosta]

yeah, I do get the feeling :/

I don't want to rely on other people to turn on auditing if they're then going to be needing my advice on the state of their cluster.

although this does rely on the fact that they turned debug on, right? (which, sure, can be turned on at runtime, which is quite useful, but is also super expensive to have it on AFAIR).

if the same was true for audit (assuming the pain-point is quickly turning the system into a more verbose mode), would you think this log message in this particular endpoint would not be needed anymore?

[cirocosta]

this caught my attention, it's something we seem to only be doing here

$  ~/workspace/concourse/atc $ ag -Q "lager.Logger" | grep counter
api/pipelineserver/archive_test.go:16://go:generate counterfeiter code.cloudfoundry.org/lager.Logger

do you think this is something we should adopt more? I'm curious about the rationale to testing the log statements here, and would challenge is a bit: if we were adding a someMetric.Add(1) (i.e., if we were incrementing a counter for something we want to measure), and even more, we had a trace that we wanted to capture, should we test those three things? log, metric, and trace?

(please don't take the above as criticism 😅 curious to know more about it)

thanks!

[jamieklassen]

I've spent a lot of time inspecting logs from Concourse with no access to the cluster. Based on this, I'm shocked by how often I really wish Concourse was logging something and I find that it's not. In general I haven't yet felt like it's necessary to unit-test every log message, but there are times and places where logging feels like a really desirable feature with real business value and I want a test to verify that it will definitely be happening - in such cases I'd be pretty upset if the log stopped happening haha.

[YoussB]

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