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

Support YAML aliases in dedicated section #32

Closed
sandro-h opened this issue Aug 10, 2017 · 8 comments
Closed

Support YAML aliases in dedicated section #32

sandro-h opened this issue Aug 10, 2017 · 8 comments

Comments

@sandro-h
Copy link
Contributor

Hey there,

YAML supports aliases out of the box. See these use cases for example, and the spec here.

As this is supported by standard yaml parsers, it's already possible to use aliases for pipeline configs to reduce duplication. For example I can do something like this:

test-win2008:
  resources:
    - win2008
  tasks:
    &test_tasks # define the whole task list as an alias
    - exec:
        command: my_build_cmd.sh
        arguments:
          - "-DConfiguration=Release"
          - "init"
    - exec:
        command: my_build_cmd.sh
        arguments:
          - "-DConfiguration=Release"
          - "build"
    - exec:
        command: my_build_cmd.sh
        arguments:
          - "-DConfiguration=Release"
          - "test"
test-win2012:
    resources:
      - win2012
    tasks: *test_tasks # use the previously defined alias
test-win7:
    resources:
      - win7
    tasks: *test_tasks # use the previously defined alias

So we use an alias to reuse the same list of tasks in multiple jobs running on different platforms.

This works well enough, but we are forced to define the alias the first time we use it somewhere in the yaml config where it is actually valid. This makes it harder to understand where aliases come from and more error prone to reorder sections in the config. It's also not possible, to my knowledge, to only define a part of a task list as an alias. In the above example, maybe I only want the first two exec tasks to be part of the alias, not the final one.

Therefore, I suggest supporting a dedicated aliases section like the one used on the first example page.
That's currently not possible because the plugin will complain about an invalid key:

cd.go.plugin.config.yaml.YamlConfigException: aliases is invalid, expected pipelines or environments

Since the yaml parser takes care of resolving aliases, I reckon it wouldn't require many changes, besides dropping the aliases section after parsing as it's no longer needed.

@tomzo
Copy link
Owner

tomzo commented Aug 11, 2017

Hi @sandro-h
I agree with most that you have said.

I suggest supporting a dedicated aliases section

I am not sure if aliases is the best way to name it, but I agree about having such section.
Since user could be storing any part of configuration there, perhaps it should be called partials or definitions or common?

I find this part especially common use case:

.. maybe I only want the first two exec tasks to be part of the alias, not the final one.

I think that could be done if we first allow tasks to be a list of (list or task). Then plugin could flatten list after parser resolves aliases. Something like this:

test-win2008:
  resources:
    - win2008
  tasks:
    &test_tasks # define the whole task list as an alias
    - exec:
        command: my_build_cmd.sh
        arguments:
          - "-DConfiguration=Release"
          - "init"
    - exec:
        command: my_build_cmd.sh
        arguments:
          - "-DConfiguration=Release"
          - "build"
    - exec:
        command: my_build_cmd.sh
        arguments:
          - "-DConfiguration=Release"
          - "test"
test-win2012:
    resources:
      - win2012
    tasks: 
      - *test_tasks # use the previously defined alias
      - exec:
        command: my_build_cmd.sh
        arguments:
          - "-DConfiguration=Release"
          - "win12"
test-win7:
    resources:
      - win7
    tasks: 
      - exec:
        command: my_build_cmd.sh
        arguments:
          - "-DConfiguration=Release"
          - "pre-win7"
     - *test_tasks # use the previously defined alias

@sandro-h
Copy link
Contributor Author

Hi @tomzo,

Thanks for the reply. I agree on both counts. I actually had the use case in mind where you want to define an alias with only a subset of tasks, but you raise a good point: if you want to use a task alias and prepend or append more tasks, then you will need to support lists of lists.

What does your roadmap look like? Do you have plans to integrate such new features yourself in the near future or would you welcome PRs?

@tomzo
Copy link
Owner

tomzo commented Aug 15, 2017

What does your roadmap look like? Do you have plans to integrate such new features yourself in the near future or would you welcome PRs?

PRs are always welcome :) and I would greatly appreciate if you would implement this.
If not I might be able to implement it sometime near the end of August. Currently I am too much involved in other projects.

@sandro-h
Copy link
Contributor Author

Sorry for the late reply. I can work on a PR but it most likely won't be ready any earlier than your suggestion due to other work ;) But I'm happy to do it either way since it shouldn't be overly time critical.

sandro-h pushed a commit to adnovum/gocd-yaml-config-plugin that referenced this issue Aug 30, 2017
sandro-h pushed a commit to adnovum/gocd-yaml-config-plugin that referenced this issue Aug 30, 2017
@sandro-h
Copy link
Contributor Author

Hello again @tomzo,

I've committed two changes to our fork:

  • support for a common section for alias definitions
  • support for nested lists in the task section

What's still missing is appropriate documentation.

I'd appreciate if you could take a look at the commits (they're hopefully visible to you as well in this issue), and tell me if they're okay like that. Specifically if we agree on the naming of the "common" section.

I can also already create a pull request, if that's easier.

@tomzo
Copy link
Owner

tomzo commented Aug 30, 2017

Hi @sandro-h,
Those 2 commits look OK. I think It might be good to add one more test with end-case of actually using aliases for tasks and whatever you wanted too.
There is a similar test here with complete yaml backing it here.

What's still missing is appropriate documentation.

I think a usage example and reference to yaml aliases spec, which you gave earlier will suffice.

Specifically if we agree on the naming of the "common" section.

"common" is fine.

I'm leaving on vacation tomorrow so I will not make any releases now. You can submit a PR when you think this is ready, I can review it and merge it when I am back, near the end of September.

Thanks for your work :)

@sandro-h
Copy link
Contributor Author

Hi @tomzo,

Agree on all counts. I wasn't fully happy with the test coverage either.

Funny, I'm leaving on vacation tomorrow as well ;) So I will do a proper PR in roughly two weeks.

Enjoy your vacation till then!

sandro-h pushed a commit to adnovum/gocd-yaml-config-plugin that referenced this issue Sep 15, 2017
sandro-h pushed a commit to adnovum/gocd-yaml-config-plugin that referenced this issue Sep 15, 2017
sandro-h pushed a commit to adnovum/gocd-yaml-config-plugin that referenced this issue Sep 15, 2017
sandro-h pushed a commit to adnovum/gocd-yaml-config-plugin that referenced this issue Sep 15, 2017
sandro-h pushed a commit to adnovum/gocd-yaml-config-plugin that referenced this issue Sep 15, 2017
sandro-h pushed a commit to adnovum/gocd-yaml-config-plugin that referenced this issue Sep 15, 2017
sandro-h pushed a commit to adnovum/gocd-yaml-config-plugin that referenced this issue Sep 15, 2017
sandro-h pushed a commit to adnovum/gocd-yaml-config-plugin that referenced this issue Sep 15, 2017
tomzo added a commit that referenced this issue Oct 16, 2017
Improve alias support with nested tasks and a common section (#32)
@tomzo
Copy link
Owner

tomzo commented Oct 25, 2017

Released with 0.6.0

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

No branches or pull requests

2 participants