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

Add support for Elm #63

Closed
joepvd opened this issue Jul 30, 2018 · 8 comments
Closed

Add support for Elm #63

joepvd opened this issue Jul 30, 2018 · 8 comments

Comments

@joepvd
Copy link
Contributor

joepvd commented Jul 30, 2018

See travis-ci/travis-build#934

What needs to be supported:

language: elm
elm_version: 1.2.3
elm_test_version: 4.5.6
node_js: 6
@joshk
Copy link
Contributor

joshk commented Jul 30, 2018

Can I suggest we look at supporting both:

elm: 1.2.3

and

elm:
  version: 1.2.3
  test: 4.5.6

@svenfuchs
Copy link
Contributor

@joshk how would that look like for matrix expansion?

elm: 1.2.3 (at least in other languages) would normalize to elm: ['1.2.3'] (an array with one value)

are you suggesting this?

elm:
  - version: 1.2.3
    test: 4.5.6
  - version: 1.2.4
    test: 4.5.7

@joshk
Copy link
Contributor

joshk commented Aug 2, 2018

@svenfuchs you are a star for pointing this out, because now i see four options, which I would like your thoughts on (we could support one or multiple of these formats):

  1. Single Elm version selection
elm: 1.2.3
  1. Single Elm and test version selection:
elm:
  version: 1.2.3
  test: 4.5.6
  1. Multi Elm and test version selection: Matrix expansion (9 jobs):
elm:
  version: [1.2.3, 1.2.4, 1.3.1]
  test: [4.5.6, 4.5.7, 5.1.1]
  1. Multi Elm and test version selection: Specific definition (2 jobs):
elm:
  - version: 1.2.3
    test: 4.5.6
  - version: 1.2.4
    test: 4.5.7

My thoughts:

  • We should do option one, simple shorthand for simple scenarios.
  • I like option two for its structure, and not polluting the root namespace.
  • I think I like option 3 because it allows for simple matrix expansion.
  • I don't think we should do option 4, as this is possible with specific Job specifications (jobs.include), but I'm open to it.

@svenfuchs
Copy link
Contributor

@joshk sorry, missed this. thanks @carlad for the hint!

@joshk i agree, i think.

essentially this is what we have with other languages except traditionally we would have had elm_version and elm_test (or elm_test_version). i do like the proposal to use a namespace instead (i'd like to namespace other things more in the future, too)

i also agree 4 wouldn't be great as this isn't something we do for other languages/things, and adding this to Gatekeeper (i.e. without travis-yml in place) would be a huge mess (on top of the huge mess that we already have), so i think i'd strongly object.

one thing to keep in mind though is that we don't have any matrix expansion keys in nested namespaces so far afaik. so that might require some additional work definitely in Gatekeeper, in travis-yml, and possibly in Web (not too sure)

@rtfeldman
Copy link

Anything I can do to help unblock the discussion?

@carlad
Copy link
Contributor

carlad commented Aug 29, 2018

@svenfuchs so just to confirm, you are also happy with option 3 that @joshk mentions?

@svenfuchs
Copy link
Contributor

@carlad yes, we're in agreement here.

we'd support all of 1-3, but not 4, either eventually or immediately. travis-yml would expand the formats 1 and 2 into the default format 3. i'm not sure if we want to do this work in Gatekeeper, too ... or require the use of travis-yml for this (there are other considerations about doing that, i guess)

also, we need to keep the caveat in mind that having an expansion key nested in a namespace is not something that they system supports out of the box. we'll have to make changes to travis-yml, Gatekeeper, and Web for this.

@BanzaiMan
Copy link
Contributor

#87

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

No branches or pull requests

6 participants