make cc.xml endpoint public, and only list public pipelines #8809

LukeWinikates · 2023-09-11T20:52:18Z

Changes proposed by this PR

closes #

make speculative code changes
ask for help
make tests pass
prove it only shows public pipelines

Notes to reviewer

Hi friends - I've never made a concourse PR before, and don't know the codebase well. I am a longtime concourse user.

I want to try out the cc.xml feature, but it requires authentication. This URL should be publicly accessible, because tools like ccmenu don't implement cookie based authentication.

I don't think that this code works quite yet, but I thought there might be value putting a sketch of the solution out there already to get advice sooner.

Release Note

Public pipelines are now accessible through the cc.xml endpoint while unauthenticated

taylorsilva

Thanks for the PR!

You'll need to add a test for this in the api test suite. You'll want to add tests to this file:

concourse/atc/api/cc_test.go

Lines 355 to 363 in d2e262b

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

You can see the current tests will need to be updated for the non-auth'd workflow.

You can run the tests by running go test ./atc/api/ from the root of the repo, where the go.mod and go.sum files are.

When I try to run the tests against your code I get this error:

┌─[feat-public-ccxml][~/workspace/concourse/concourse]
└─▪ go test ./atc/api/
atc/api/ccserver/get.go:4:2: missing go.sum entry for module providing package code.cloudfoundry.org/lager (imported by github.com/concourse/concourse/atc/api/ccserver); to add:
        go get github.com/concourse/concourse/atc/api/ccserver

Actually not sure if this dependency should be changed or not 🤔 Should probably still be "code.cloudfoundry.org/lager/v3". Not sure if your IDE made these suggestions?

I'll leave a few other comments, but adding tests is the big thing that needs to happen here.

atc/api/ccserver/get.go

LukeWinikates · 2023-09-12T17:03:38Z

Thanks @taylorsilva !

LukeWinikates · 2023-09-12T18:50:40Z

@taylorsilva how would you expect this feature to behave for teams with no public pipelines? 404? 401? 200 with empty XML?

taylorsilva · 2023-09-13T16:45:53Z

@LukeWinikates 200 with empty xml seems to make the most sense at first glance.

I decided to do some research to see if anyone ever did anything like this before. Came across this comment: #5308 (comment)
Which goes over a broader idea on how to share the status of pipelines/jobs. Way out of scope in this PR, don't change what you're doing here!

The original PR that added cc.xml has some interesting tidbits about the auth stuff: #2759
Seems like other cc.xml-based products support auth and CCMenu is just one that doesn't. How interesting!

taylorsilva

looking good so far! Just add some tests, run go fmt and go mod tidy to clean some stuff up.

atc/api/ccserver/get.go

go.mod

atc/api/cc_test.go

LukeWinikates · 2023-09-18T20:38:29Z

I decided to do some research to see if anyone ever did anything like this before. Came across this comment: #5308 (comment) Which goes over a broader idea on how to share the status of pipelines/jobs. Way out of scope in this PR, don't change what you're doing here!

Thanks for digging! Using the public pipeline concept to determine what cc.xml shows makes intuitive sense to me. It seems as though vito was introducing an idea of uuids so that the badge/cc.xml URL wouldn't reveal the team name, but I'm not sold on the value of that.

The original PR that added cc.xml has some interesting tidbits about the auth stuff: #2759 Seems like other cc.xml-based products support auth and CCMenu is just one that doesn't. How interesting!

I'd be interested in alternatives to CCMenu, but I haven't had luck finding one. We are having trouble managing Slack notifications as a way of observing build errors, and this was the next thing I thought to try. I couldn't find a different cc.xml client for Mac with working install instructions.

Signed-off-by: Luke Winikates <winikatesl@vmware.com>

LukeWinikates · 2023-09-21T16:43:01Z

@taylorsilva I just force-pushed this to bring it up to date with master and remove a go.mod change that didn't need to be in there that was causing a merge conflict. I don't expect any more changes. 🎩

LukeWinikates · 2023-09-28T20:09:30Z

@taylorsilva is there anything else I can do to help get this PR ready to merge?

taylorsilva · 2023-09-28T20:16:14Z

@LukeWinikates sadly not, unless you can make more time in the day for me to get stuff done haha 😅 I had to give a couple interviews at work this week, so my time has been eaten up.

All I want to do is make sure the tests are behaving the way we expect them too and then this will be good to merge. Thanks for your patience!

Signed-off-by: Taylor Silva <dev@taydev.net>

taylorsilva · 2023-10-02T16:02:38Z

Merged! Thanks again for the PR @LukeWinikates

LukeWinikates · 2023-10-02T19:03:11Z

thanks @taylorsilva

LukeWinikates requested a review from a team as a code owner September 11, 2023 20:52

LukeWinikates force-pushed the feat-public-ccxml branch from a2ee95e to dc62930 Compare September 11, 2023 21:54

taylorsilva added the enhancement label Sep 12, 2023

taylorsilva requested changes Sep 12, 2023

View reviewed changes

atc/api/ccserver/get.go Outdated Show resolved Hide resolved

LukeWinikates force-pushed the feat-public-ccxml branch from fa8b6fc to 2f0e7bc Compare September 12, 2023 18:48

LukeWinikates requested a review from taylorsilva September 12, 2023 18:58

taylorsilva requested changes Sep 13, 2023

View reviewed changes

atc/api/ccserver/get.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

atc/api/cc_test.go Outdated Show resolved Hide resolved

LukeWinikates force-pushed the feat-public-ccxml branch from 5b0ea8b to 2366de5 Compare September 18, 2023 20:13

LukeWinikates requested a review from taylorsilva September 18, 2023 20:38

LukeWinikates closed this Sep 21, 2023

LukeWinikates force-pushed the feat-public-ccxml branch from 2366de5 to 0d95003 Compare September 21, 2023 16:16

LukeWinikates reopened this Sep 21, 2023

make cc.xml endpoint public, and only list public pipelines

90546a1

Signed-off-by: Luke Winikates <winikatesl@vmware.com>

LukeWinikates force-pushed the feat-public-ccxml branch from 31ca927 to 90546a1 Compare September 21, 2023 16:40

nit: go fmt

610b1fe

Signed-off-by: Taylor Silva <dev@taydev.net>

taylorsilva approved these changes Oct 2, 2023

View reviewed changes

taylorsilva merged commit c2bdd24 into concourse:master Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

make cc.xml endpoint public, and only list public pipelines #8809

make cc.xml endpoint public, and only list public pipelines #8809

Uh oh!

LukeWinikates commented Sep 11, 2023 •

edited by taylorsilva

Loading

Uh oh!

taylorsilva left a comment

Uh oh!

Uh oh!

LukeWinikates commented Sep 12, 2023

Uh oh!

LukeWinikates commented Sep 12, 2023

Uh oh!

taylorsilva commented Sep 13, 2023 •

edited

Loading

Uh oh!

taylorsilva left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LukeWinikates commented Sep 18, 2023

Uh oh!

LukeWinikates commented Sep 21, 2023

Uh oh!

LukeWinikates commented Sep 28, 2023

Uh oh!

taylorsilva commented Sep 28, 2023

Uh oh!

taylorsilva commented Oct 2, 2023

Uh oh!

LukeWinikates commented Oct 2, 2023

Uh oh!

Uh oh!

	Context("when not authenticated", func() {
	BeforeEach(func() {
	fakeAccess.IsAuthenticatedReturns(false)
	})

	It("returns 401", func() {
	Expect(response.StatusCode).To(Equal(http.StatusUnauthorized))
	})
	})

Uh oh!

make cc.xml endpoint public, and only list public pipelines #8809

make cc.xml endpoint public, and only list public pipelines #8809

Uh oh!

Conversation

LukeWinikates commented Sep 11, 2023 • edited by taylorsilva Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed by this PR

Notes to reviewer

Release Note

Uh oh!

taylorsilva left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LukeWinikates commented Sep 12, 2023

Uh oh!

LukeWinikates commented Sep 12, 2023

Uh oh!

taylorsilva commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taylorsilva left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LukeWinikates commented Sep 18, 2023

Uh oh!

LukeWinikates commented Sep 21, 2023

Uh oh!

LukeWinikates commented Sep 28, 2023

Uh oh!

taylorsilva commented Sep 28, 2023

Uh oh!

taylorsilva commented Oct 2, 2023

Uh oh!

LukeWinikates commented Oct 2, 2023

Uh oh!

Uh oh!

LukeWinikates commented Sep 11, 2023 •

edited by taylorsilva

Loading

taylorsilva commented Sep 13, 2023 •

edited

Loading