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

RFC: set_pipeline step #31

Merged
merged 7 commits into from
Jun 24, 2020
Merged

RFC: set_pipeline step #31

merged 7 commits into from
Jun 24, 2020

Conversation

vito
Copy link
Member

@vito vito commented Jul 11, 2019

Rendered

Please comment on individual lines, not at the top-level.

@evanchaoli
Copy link
Contributor

@vito Please consider to support pipeline self set. For example, for the set_pipeline step, if configure pipeline-name: self, where self is a reserved word, the it just do self set. With self set, pipeline name and team are known from the current pipeline.

I see you have create issue #4254 to track implementation of set_pipeline. Once this step is in place, it will simplify a lot of our use cases. I think set_pipeline step could be done on ATC, just call same function as current set-pipeline API, no need to start any container. If that's true and you are ok, I can contribute a PR, as we eager for this feature.

Comment on lines 71 to 84
Pros:

* Fairly straightforward semantics which seem to support a natural follow-up
question after learning about the `set_pipeline` step.
* Keyword use has precedent in `version: every`/`version: latest`/`inputs: all`.

Cons:

* Supporting both self-updating pipelines and projects could cause confusion
and fragmentation; it doesn't seem wise to have two competing approaches to
the same goal.
* Given that `self` isn't *critical* (it's easy to work around through
templating, i.e. `set_pipeline: ((name))`), is it worth the
risk/maintenance?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have multiple teams and 100s of pipelines coming from multiple git repos. We are addressing fly sp workflow for all changes to pipelines ymls with pipeline_resource and/or a custom built resource with fly binaries listening on these pipelines files/folders.
Even though we do not have strong preference for using either pipeline name or self as set_pipeline values( Good that we support both :) ), we are really excited for the set_pipeline plan. We also go through lot of compliance/audit related activities periodically and pipeline config changes being tracked in the same git repo as the pipeline definitions will help a lot.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a relatively small set of pipelines, so both ways are ok for us. Honestly, I like the self build in reference it is clean, straightforward, and easy to deal with.

evanchaoli added a commit to evanchaoli/concourse that referenced this pull request Dec 6, 2019
This is a follow-up change of PR#4708 to add "set-pipeline: self".
The RFC is concourse/rfcs#31.

Signed-off-by: Chao Li <[email protected]>
@osis
Copy link
Member

osis commented Jan 14, 2020

Weird error...

unknown artifact source: '.' in file path './repo/ci/pipeline.yml'

changing the file path to repo/ci/pipeline.yml fixes it.

@TheDevMinerTV
Copy link

TheDevMinerTV commented Mar 20, 2020

Works so far, although I would like to see the team name to be able to be changed. Right now I have four teams which all have their own auto-setting pipeline, which I would like to merge into one.

@DanielJonesEB
Copy link

Please comment on individual lines, not at the top-level.

Err, that guidance aside...

The old pipeline resource accepted globs in its vars_files elements. The new step doesn't. I'm not sure I can make a compelling case for accepting more generic input instead of specific input, but it is a bit more hassle if you've got a versioned file being used as a vars file. In our use case we have to use Minio rather than S3, so we have to have a version number in the file name of assets we track.

I thought I'd flag it in case this is an unintentional reduction in functionality.

@vito
Copy link
Member Author

vito commented May 7, 2020

@DanielJonesEB Interesting, definitely not intended. Good call! That worked previously because the resource itself performed glob expansion against its local filesystem. For the set_pipeline step that will be a bit more challenging, as we don't have access to a local filesystem. Hmmm. 🤔 I'm going to record that as an open question.

@osis Ack, hmm, that's a silly bug, but funnily enough kind of related to the above problem too. 🤔 🤔 🤔 I wonder if we need some sort of pseudo-filesystem abstraction.

@TheDevMinerTV That's a tricky one. Configuring within the team is somewhat easy because we can assume someone set the configuring pipeline within that team and therefore they have authority to set other pipelines. But once you go cross-team, that same assumption is no longer valid - only if you're the main team, which feels like a pretty limited use case.

I think this touches on a larger goal of full-stack automation, which might involve things like a set_team step and perhaps even a set_project step (assuming #32) in the future. I think for now it's best that we leave that out of scope of this RFC, but it'd be a super interesting discussion to hold elsewhere. Until we hash that out I don't think we can/should have a set_pipeline step that can configure in other teams.

@TheDevMinerTV
Copy link

Sounds fair 😄

@xtremerui
Copy link
Contributor

Coming from concourse/concourse-pipeline-resource#10, is set_pipeline step gonna support destroy or there will be a destroy_pipeline step in the roadmap?

@vito
Copy link
Member Author

vito commented May 20, 2020

@xtremerui I don't think we'll need destroying with the advent of pipeline archiving, where you can just remove the step from you config.

@vito vito merged commit 6e5ba87 into concourse:master Jun 24, 2020
@vito vito deleted the set-pipeline-step branch June 24, 2020 19:07

For example, the step should only ever configure one pipeline at a time - it should not support the `pipelines:` functionality for configuring a bunch at once.

Similarly, the step should not support fully dynamic configuration (`pipelines_file:`).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi !
I'd like to to know which part of 'concourse-pipeline-resource' dynamic configuration do you plan to implement ? It could be great to have all "keys" (var_files, team, config_file, vars, unpaused and exposed) except as you said pipelines.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, vars and var_files are implemented, config_file is the same as file:, unpaused is just forced as true, team is experimentally supported (collecting feedback here: concourse/concourse#5731), and exposed has just been proposed (https://github.com/concourse/rfcs/discussions/75).

Copy link

@o-orand o-orand Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for answering!
I should have miss something about dynamic configuration of set_pipeline. For me, dynamic config was like file for task (https://concourse-ci.org/config-basics.html#schema.file-path), or https://github.com/concourse/concourse-pipeline-resource#dynamic for a single pipeline. So, is it possible to generate set_pipeline config using previously executed task, for instance to select var_files to apply ?

@gid0
Copy link

gid0 commented Oct 27, 2020

Is it planned to implement a get_pipeline step in the future, to do what the concourse-pipeline-resource was doing as "in"?

@vito
Copy link
Member Author

vito commented Nov 5, 2020

@gid0 There are no plans for this at the moment. If you've got a use case for this, feel free to open a new discussion!

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

Successfully merging this pull request may close these issues.