Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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: Pipeline config #567

Closed
5 of 7 tasks
6543 opened this issue Dec 4, 2021 · 42 comments
Closed
5 of 7 tasks

RFC: Pipeline config #567

6543 opened this issue Dec 4, 2021 · 42 comments
Labels
breaking will break existing installations if no manual action happens feedback this needs feedback pipeline-config related to pipeline config files (.yaml) summary it's a summary for lot of issues

Comments

@6543
Copy link
Member

6543 commented Dec 4, 2021

Lets collect ideas for the pipeline config format. 🚀

We should have a critical look at the current config format and propose all possible changes to the format with the following aspects in mind:

  • allowing new users to easily understand configs and write own one (=> exp. one way to solve a problem)
  • be as generic and extendable as possible
  • stick to standards to allow the usage of (parsing) libraries with minimal additional code required

General feature requests for the pipeline (can be implemented in the current version already if possible)

from #393

Current idea:

Version could be used to run pipeline parsing with some kind of sub-program for that specific version.

Backend data format / type should not depend on docker types. For example we should not have networks as a property in steps.

version: 2

depends_on:
  - test
  - web

when:
  - branch: main
    event: push
    path: docs/
  - event: pull_request
    path: docs/

steps:
  - name: Test
    plugin: test-java:next
  - name: Build
    image: node:16-alpine
    commands:
      - npm run build
    depends_on:
      - Test
@6543 6543 added the summary it's a summary for lot of issues label Dec 4, 2021
@6543 6543 added this to the 0.16.0 milestone Dec 4, 2021
@6543 6543 pinned this issue Dec 4, 2021
@6543 6543 unpinned this issue Dec 20, 2021
@6543 6543 pinned this issue Dec 28, 2021
@LamaAni

This comment was marked as off-topic.

@6543

This comment was marked as off-topic.

@LamaAni

This comment was marked as off-topic.

@6543 6543 changed the title Breaking changes before releasing 0.16.0 Breaking changes before releasing v1.0.0 Feb 5, 2022
@anbraten anbraten changed the title Breaking changes before releasing v1.0.0 RFC: Pipeline config Feb 20, 2022
@anbraten anbraten added breaking will break existing installations if no manual action happens discussion feedback this needs feedback pipeline-config related to pipeline config files (.yaml) and removed summary it's a summary for lot of issues labels Feb 20, 2022
@anbraten anbraten mentioned this issue Mar 1, 2022
3 tasks
@LamaAni

This comment was marked as off-topic.

@LamaAni
Copy link
Contributor

LamaAni commented Mar 3, 2022

My recommendation:

when:
  - ...

images:
  runner: ubuntu:20
  maker: golang:latest

# All types show echo the yaml structure, and no "special"
# Handling. This allows for expansion without "custom" knowlage.
steps:
  - name: run
    runs_on: runner
    commands:
      - echo ok
  - name: make
    run_on: maker
    environment:
      A: b
      C: d
    secrets:
      A: b
      C: d
    commands:
      - make

Following the principles:

  1. All types should echo the yaml structure, and no/minimal "special" handling.
  2. Least amount of custom knowhow. WYSIWYG. (For example when is list. Always).
  3. Dictionaries (like envs) should either be a Name: Value, of list of values, but always following yaml standards. e.g.
     environment:
       - name: a
         value: a
     # or
     environment:
       A: b
       C: d
     # But never
     environment:
       - A=b
       - C=d
    This would allow for use of anchors properly in values.
  4. We should stick to kubernetes container definition, since everyone knows it and it proved easy over the years. You can find it in the pod help here

@6543
Copy link
Member Author

6543 commented Mar 3, 2022

we can but it would break all existing configs, what's the benefit?

you can add it to #829 as comment, but for now I don't see a benefit - if it would technically improve a ting or make something possible that was not before it should be considered - but just for the sake of taste we should not break things - call it backwards compatibility

@anbraten
Copy link
Member

anbraten commented Mar 3, 2022

All types should echo the yaml structure, and no/minimal "special" handling.
(For example when is list. Always).

👍🏾 I really like that idea of having just one way of doing things. Having the option to use arrays or just a single string confuses be quite often and makes things like json-schema a hell to write.

@LamaAni
Copy link
Contributor

LamaAni commented Mar 3, 2022

It would. When we are following the yaml config we can do the following things:

  1. Add default definition to a server, E.G. Allow a "default pipeline" that will be merged.
  2. Easier for new people to understand.
  3. Allow anchors to be used in the yaml config anywhere -> leads to more configurable yaml.

In general, the idea of version is good since it allows one to slowly migrate to a more "readable" yaml. Following standards, IMO, is the best way to expand use.

Finally, Once proper yaml exists, we can in general start considering dynamic pipelines.

@LamaAni
Copy link
Contributor

LamaAni commented Mar 3, 2022

Oh another thing, one can see an example of over complication by not following yaml standards in the Containers unmarshel,

	for i, n := range value.Content {
		if i%2 == 1 {
			container := Container{}
			err := n.Decode(&container)
			if err != nil {
				return err
			}

			if container.Name == "" {
				container.Name = fmt.Sprintf("%v", value.Content[i-1].Value)
			}
			c.Containers = append(c.Containers, &container)
		}
	}

Instead of

containers := []*Container{}
err := value.Decode(&containers)

Which complicates the code and causes errors. Makes things harder to manage or write.

@anbraten anbraten removed this from the 1.0.0 milestone Mar 3, 2022
@anbraten anbraten added the summary it's a summary for lot of issues label Mar 3, 2022
@anbraten
Copy link
Member

anbraten commented Mar 3, 2022

To be able to have multiple parser (for different versions) in Woodpecker I would like to do some changes to the parsing (exp. creating a clean interface what needs to be inserted and whats the output). I already started that a bit in #802.

@anbraten anbraten mentioned this issue Apr 4, 2022
31 tasks
@silverwind
Copy link
Contributor

silverwind commented Oct 18, 2022

Two points from me:

  1. Allow to define steps in array format (as in OP), to avoid undefined dict key iteration problems with YAML parsers. yaml pipeline vs. 2 #771 (comment).
  2. Support parallelism inside a pipeline via a DAG. This requires a way to define step dependencies which can then allow the runner to build said DAG. Edit: I see group provides something similar for static fan-out, but certainly not as flexible as defining dependencies directly.

@6543
Copy link
Member Author

6543 commented Oct 18, 2022

  1. is already resolved

@gapodo
Copy link
Contributor

gapodo commented Nov 2, 2022

Another thing (should I open FRs for these ideas?) would be some kind of badge_groups[].name in the step, allowing to collect success-statuses of various parts of the pipeline, previously I would have said just abuse the group (i.e. showing the Status as: Linting: success, Binary Builds: success, Container Builds: failing).

This might be augmented with something like globalStatusEnabled and badge_groups[].statusEnabled to allow one to ignore the return of a step for the overall or badge success status.

apiVersion: woodpecker-ci.org/v2
kind: pipeline

steps:
  - name: Lint
    badge_groups:
      - name: Lint
      - name: ContainerBuild
      - name: BinaryBuild
      - name: Docs
        statusEnabled: false

  - name: ContainerBuild-debian
    badge_groups:
      - name: ContainerBuild

  - name: ContainerBuild-alpine
    badge_groups:
      - name: ContainerBuild

  - name: BinaryBuild
    badge_groups:
      - name: BinaryBuild

  - name: DocsBuild
    badge_groups:
      - name: DocsBuild

  - name: notify
    globalStatusEnabled: false
    badge_groups:
      - name: Notification

I've left out images,... just to show the basic idea. This would give us 5 specific badges + the global one, where the global one ignores the notify step (we've built successfully but not notified).

@genofire
Copy link

I am missing in the list above:
Make woodpecker code reusable in multiple projects, like #1089 (or #1179 gitlab-ci include or renovatebot shareable config https://docs.renovatebot.com/config-presets/)

@6543
Copy link
Member Author

6543 commented Dec 23, 2022

Another thing (should I open FRs for these ideas?) would be some kind of badge_groups[].name in the step, allowing to collect success-statuses of various parts of the pipeline, previously I would have said just abuse the group (i.e. showing the Status as: Linting: success, Binary Builds: success, Container Builds: failing). ...

that looks like a reimplementation of workflows within a workflow :/

@gapodo
Copy link
Contributor

gapodo commented Dec 23, 2022

that looks like a reimplementation of workflows within a workflow :/

somewhat looks like it, though the goal isn't to implement a workflow, but to collect states to allow for badges / reporting on parts of the entire workflow (think status page, linting ok, security ok, binary-build ok, docker-build failed, integration testing failed).

But I know that this would be a "polish" feature, not a base thing / necessity (though collecting that info can be a bit ugly)

@lafriks
Copy link
Contributor

lafriks commented Dec 24, 2022

Could be nice to add way to import pipelines from external repos (similarly how go imports packages):

import:
  - github.com/org/repo/dir/pipeline.yaml
  - git.mycomany.com/org/repo/build.yaml

@gapodo
Copy link
Contributor

gapodo commented Dec 25, 2022

Could be nice to add way to import pipelines from external repos (similarly how go imports packages):

import:
  - github.com/org/repo/dir/pipeline.yaml
  - git.mycomany.com/org/repo/build.yaml

I'm unsure on this, it may be more suitable to lightly restructure the yaml and use yaml templating (could still be an import), this would allow including (named) templates which may either be used as is, or have single (or multiple) values overridden.

The one problem I see with imports in general is the security risk associated with including "something" (we should at least make it possible to either add checksums, or tags,... also disabling it on a per server basis would be nice (or having a allow list for imports))

Edit: just to add this may get dangerous quickly if the import just changes to including some secret, and uploading it to somewhere...

@genofire
Copy link

genofire commented Dec 25, 2022

To minimize the risk, it would be nice to set an version (git commit id or an git Tag). In the end, you should just import own developed or verified by an code review ... (like on all Software components you use).

I am not sure if templating is needed, to parse values, environment variables are enought on gitlab-ci.


@lafriks maybe also relativ, like /org/repo/dir/pipeline.yaml for easier move to a new git hosting platform.

@gapodo
Copy link
Contributor

gapodo commented Dec 25, 2022

maybe also relativ, like /org/repo/dir/pipeline.yaml for easier move to a new git hosting platform.

I'd tend to not use the relative path as a "git instance" path, if I'd use it at all I'd go with repo relative (allowing reuse within the repo, which may get useful when running pipelines for multiple arches, or having standard prepare steps, that are used in PR pipelines as well as test and release...)

Regarding templating, I'm fairly sure it would be the way to go, as you could import, and use the step at multiple places, reconfiguring it as you need it (changing names, just the command, the environment,...). Gitlab-CI which @genofire used as an example handles variables completely differently and in their context you can really achieve most things that way (though also not all).

@6543
Copy link
Member Author

6543 commented Dec 25, 2022

If we gona introduce imports and templates ...
two questions:

@6543
Copy link
Member Author

6543 commented Dec 25, 2022

To minimize the risk, it would be nice to set an version (git commit id or an git Tag). In the end, you should just import own developed or verified by an code review ... (like on all Software components you use).

it we really wana make import secure, we would have to come up with something similar to how go-modules, rust crates etc... are handled :/
-> a trusted version/hash database which woodpecker do have to check against ... and some lock mechanism

@6543
Copy link
Member Author

6543 commented Dec 25, 2022

two questions: should templates at max contain a whole pipeline, only a workflow or only steps
because as soon as it should cover more than steps we have tradeoffs:

  • workflows -> we have to overwrite all other things of a workflow to get a predicted behavior
  • pipelines -> well in that case I would revere to RFC: pipeline compile step #1400 totaly

@gapodo
Copy link
Contributor

gapodo commented Dec 25, 2022

Cross-referencing #1504 as another proposal for the Pipeline config... (setting up envs in one step for use in another one, that may also be a plugin)

@6543
Copy link
Member Author

6543 commented Dec 25, 2022

what a pure base config not become -> scriptable (turing-complete)

that's what isolated environments are for ...

@gapodo
Copy link
Contributor

gapodo commented Dec 25, 2022

two questions: should templates at max contain a whole pipeline, only a workflow or only steps because as soon as it should cover more than steps we have tradeoffs:

* workflows -> we have to overwrite all other things of a workflow to get a predicted behavior

* pipelines -> well in that case I would revere to [RFC: pipeline compile step #1400](https://github.com/woodpecker-ci/woodpecker/issues/1400) totaly

With imports I'm a bit unsure, to me I'd tend to go with the step as unit of inclusion (and potential templating, which BTW is a native yaml feature), I can see use-cases, where including more than a single step (potentially even a complete pipeline) could be useful though (think standard build pipeline, that just gets a few values set, e.g. debug-runs and release runs)

@6543
Copy link
Member Author

6543 commented Dec 25, 2022

well if its so generic, it can be covered by a plugin ... (and/or) that geneates the pipeline ... and so #1400 would cover it without adding complexity to woodpecker itselve

@gapodo
Copy link
Contributor

gapodo commented Dec 25, 2022

what a pure base config not become -> scriptable (turing-complete)

that's what isolated environments are for ...

Is this a reference to #1504 or the templating / imports discussion?

If it's re #1504, I'd not call it scriptable, it's more of a way to prepare / setup an environment, which would either need to be repeated often within the pipeline, or is used by a step, that is not capable of setting up the environment as needed (think plugins)

@gapodo
Copy link
Contributor

gapodo commented Dec 25, 2022

For the import step, I'd agree that it is possible to do this in a compile step (if that gets implemented) as for yaml templating it's a native yaml feature, that would "just" need a slight refractor of the pipeline config to allow for it (it's mostly, that we name the step by naming the root node instead of having step as the root for a step definition and a step.name inside it)

@gapodo
Copy link
Contributor

gapodo commented Dec 25, 2022

And I need to add a correction the templating via merge keys (which I was referring to when talking about templating) is implemented in many yaml-implementations, but isn't in the yaml1.2 spec... (due to a "conflict" with one of the yaml core folks) sorry if I caused confusion...

It's documentation is here... https://yaml.org/type/merge.html

Also the go-library used by woodpecker (gopkg.in/yaml.v3) supports it. It's implemented in the decode.go (tested in decode_test.go) and is part of resolve.go.

@6543
Copy link
Member Author

6543 commented Dec 25, 2022

ah ... that's already on the todo list -> #1192

@gapodo
Copy link
Contributor

gapodo commented Dec 25, 2022

ah ... that's already on the todo list -> #1192

ah sorry didn't notice that one... (it would play nicely with templating though)

@6543

This comment was marked as resolved.

@6543
Copy link
Member Author

6543 commented Jun 6, 2023

add version to workflow configs -> #1834

@woodpecker-ci woodpecker-ci locked and limited conversation to collaborators Aug 20, 2023
@anbraten anbraten converted this issue into discussion #2255 Aug 20, 2023
@anbraten anbraten unpinned this issue Aug 20, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
breaking will break existing installations if no manual action happens feedback this needs feedback pipeline-config related to pipeline config files (.yaml) summary it's a summary for lot of issues
Projects
None yet
Development

No branches or pull requests

9 participants