-
-
Notifications
You must be signed in to change notification settings - Fork 867
feat(atc): allow global setting to expose badges for hidden pipelines #5308
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
Conversation
@pivotal-jamie-klassen here is the initial cut at the ATC flag implementation for the badges. The |
This change reworks the prior fix in concourse#5252, by providing a flag for ATC to set the default badge visibility. The default setting for `default-badge-visibility` is for it to maintain backwards compatible behavior (`pipeline-visibility`), while the `public` setting makes all pipeline and job badge routes public by default. Fixes concourse#996. Signed-off-by: Joseph Hosteny <jhosteny@carnegierobotics.com>
66fc392
to
c7eb3b7
Compare
Signed-off-by: Joseph Hosteny <jhosteny@carnegierobotics.com>
@jhosteny Sorry, but I bring bad news: in order to get 6.0 out the door I'm going to revert your original PR (#5252) for now. In talking with the team we've started thinking a bit more about this problem space and we'd like to continue developing the idea, possibly in a different direction from this PR, so that it will make more people in the community happy. Since that'll take a bit more time and since 6.0 has already been delayed for a while I'd like to just revert the other change for now to de-risk the release a bit. Sorry to have strung you along. 🙁 So, here's what we're thinking: if we make these status endpoints (both badge and Something like this: $ fly publish-status --team some-team
badge: https://ci.example.com/api/v1/status/<guid>/badge
cc.xml: https://ci.example.com/api/v1/status/<guid>/cc.xml
$ fly publish-status --pipeline my-pipeline
badge: https://ci.example.com/api/v1/status/<guid>/badge
cc.xml: https://ci.example.com/api/v1/status/<guid>/cc.xml
$ fly publish-status --job my-pipeline/my-job
badge: https://ci.example.com/api/v1/status/<guid>/badge
cc.xml: https://ci.example.com/api/v1/status/<guid>/cc.xml ...and I haven't really thought further than that. 😅 There may be some way to delete and list them. This would accomplish the following goals:
I'm leaning towards this approach because it will allow for greater flexibility while not having to make the decision cluster-wide. We really want to limit the amount of globally-configured flags because many instances of Concourse are multi-tenant, and while one team may want their badges publicly exposed, other teams may have security concerns that prohibit it. I don't meant to expand the scope on you, but since this area has been a recurring pain point for our users I'd like to take the opportunity to discuss it more in-depth. We'd be happy to help guide along these changes or if you feel that the scope has increased beyond what you have time for, we can try to put together an RFC so that someone in the community may volunteer. Let me know what you think! 🙏 |
@vito no worries, I understand. I think this probably needs some community feedback. My first reaction is that the GUID is slightly inconvenient. The badge URL is not self-describing, which means that I need to publish the status before I can update a README, for example, and I would invalidate a consumer of the endpoint if I unpublished / republished. Perhaps you could have a One other thing to consider is that you'd have to still maintain the old badge routes. Would the existing pipeline / job badge route functionality get extracted to new status servers, that the old routes would redirect to if public or authenticated / authorized, and the GUID based routes would redirect to dynamically? My personal preference would be setting an option on the pipeline or job configuration that made it explicit that the badge is public, but I understand this is at odds with the CLI for control of pipeline visibility. Or, alternatively, the badge authentication route handler could check the DB to see if it had been exposed, so the route is still the same and there is no global flag (a DB migration would set the default scope for backwards compatibility). Regardless, I'd be willing to help out on this, but I expect that it would take a fair bit longer to implement given that I'm pretty new to the codebase. |
Thanks for understanding. 🙂
Hm yeah true. Either way though there's some sort of operation you have to do prior to updating the README; if it's not creating a badge, it'd be enabling the badge endpoint. But you're right that a GUID in the URL would require you to do so before committing the change. One advantage of having an absolute, anonymous URL is that the URL could survive pipeline/job renaming and pipeline ownership-changing (assuming we implement I guess it ultimately comes down to which event happens more often: renaming/moving pipelines/jobs or deleting badges. My gut tells me the former, since badges are relatively set-and-forget, but pipelines and jobs tend to evolve and sometimes need to be reorganized or renamed. But it is true that we would need to be conscious of operations which would invalidate these URLs. So we would at least need a way to update a status's configuration in-place instead of deleting and re-creating.
True. Yeah, that sounds like it'd be a reasonable approach. I think they could both ultimately end up going down the same code path. The GUID based routes could load the pipeline/job status based on the badge's config in the database, and the legacy routes would just get them from the URL. And then they both could call the same bit of code for actually returning the response.
This actually wouldn't be a bad idea either. We already have a notion of whether a job is To view the build you would need to run |
I think I intended that just the badge itself would be public, which means it would be a separate setting. |
Hi @jhosteny! I like your idea of having this be a setting on the job/pipeline rather than as a global flag setting. It would be pretty similar to how you can make a job public through the One question I would have about this approach would be about making a private pipeline have public badges. If we make it so that you can make a private job have public badges through a setting in it's config, would we need a similar approach for making a private pipeline have public badges? Not sure how that would look, would it need a fly CLI for it similar to I would be willing to help you through implementing this new approach with adding a new job configuration for public badges if you would be interested in it! If you are unsure about where to start within the codebase I can definitely help with that too. 😄 |
Hi @clarafu, I'm sorry for the long delay in responding. I am definitely interested in helping on this, but I am a bit behind at work right now and haven't had the time to start. I also need some time to review the existing codebase to think about this added functionality will work. Now that I am actually starting to move pipelines between teams, I better understand @vito's comments regarding The specific use case for us is that all pipelines / jobs are private by default. I want only the badge (no logs, or any other info) to be public, just so we can have a badge in the README on GH, for example. I think a perfectly reasonable first cut at this doesn't allow the jobs to override the pipeline setting - they get the same badge visibility setting as the pipeline badge. |
@jhosteny Don't worry take all the time you need :) If you ever have any questions or get stuck on any part of the codebase, please don't hesitate to ask us! I'm wondering if we should have the pipeline badge commands be similar to setting a pipeline config or our This also seems like it can be a bigger discussion which can be in the format of an RFC. There are quite a few pieces here and it might be better to be able to clearly discuss how we are going to approach this. If you haven't taken a look at our existing RFCs before, they live here! |
Hi @jhosteny! I'd like to close this PR for now because it has been almost one year of inactivity but feel free to reopen this PR or open a new one once you find time to tackle it! Thanks :) |
This change reworks the prior fix in #5252, by providing a flag for ATC
to set the default badge visibility. The default setting for
default-badge-visibility
is for it to maintain backwards compatiblebehavior (
pipeline-visibility
), while thepublic
setting makes allpipeline and job badge routes public by default.
Fixes #996.
Signed-off-by Joe Hosteny jhosteny@carnegierobotics.com
Existing Issue
Fixes #996.
Why do we need this PR?
PR #5252 contained an initial implementation for this functionality, but it did so in a backwards incompatible manner. This fix keeps the default behavior, but allows it to be overridden with a new ATC flag.
Changes proposed in this pull request
default-badge-visibility
with a default setting ofpipeline-visibility
Contributor Checklist
Reviewer Checklist
and Helm packaging; otherwise, ignored for the integration tests (for example, if they are Garden configs that are not displayed in the
--help
text).