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

Allow setting os, dist, and language keys and defaults for them #62

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

seankelly
Copy link
Member

This is a fix for #32. I've been running it branched off 0.3 for months and rebased it off master and submitted. No idea yet if tests pass.

The gist is the master-side Travis config can set options for the os, dist, and language keys such as properties for the build to inherit. Defaults are also possible to set so dist can be something other than precise.

@mention-bot
Copy link

@seankelly, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Jc2k, @tardyp and @rodrigc to be potential reviewers.

@seankelly seankelly force-pushed the upstream-job-options branch 2 times, most recently from d2986dc to 01e50d5 Compare April 25, 2017 20:51
@tardyp
Copy link
Member

tardyp commented Apr 26, 2017

I would like to have a README paragraph about this in order to better understand how this is supposed to work

@tardyp
Copy link
Member

tardyp commented Apr 26, 2017

thanks for documenting it. I missed the job_options feature, which I like.
but I wonder if this is not a little bit too specific.

os and dist are imho not fitting very well to the matrix concept.
You won't need to generate all combination of os and dist, as win + debian_7 does not make sense.
while win + python / win + c makes sense for a matrix.

people might want to select different images based on other criterias.

os: linux_debian_7, langage=c, db=mysql => image=debian_mysql_gcc

so I think it would make more sense to have something like:

automatic_properties:
  image:
    - criterias:
         dist: linux_debian_7
         langage: c
         db: mysql
       value: debian_mysql_gcc 
     - # default
       value: default_builder 
  os:
    - criterias:
         dist: windows
      value: windows
    - value: linux

obviously this makes the implementation much more complicated than the 4 lines you have :-}

@seankelly
Copy link
Member Author

win + debian_7 wouldn't be possible because it's hierarchical. The debian_7 would only appear under linux. A more complete example:

job_options:
  linux:
    debian_7:
      ...
  win:
    win_10:
      ..

Definitely can't handle looking up based on other criteria however, such as database. Might be able to do something without too much code. Perhaps select the criteria with the most parts that match, with the first to have that many matches winning.

@seankelly
Copy link
Member Author

This PR sort of does two things:

  1. Set dist, os, and language as properties along with making it more flexible beyond assuming language=python.
  2. Looks up those properties to set other properties. The job_options part.

I've updated my internal version such that the second option is no longer needed. Because of that, if I remove that, would it be possible to merge this? I still need the dist, os, and language properties set.

@tardyp
Copy link
Member

tardyp commented Aug 8, 2017

I dont see update in the code? did you forget to push?

@seankelly
Copy link
Member Author

I haven't updated the code yet, it was a hypothetical question.

@seankelly
Copy link
Member Author

Did the code change anyway. I think I removed the job_options part. It was much smaller than the rest.

@seankelly seankelly mentioned this pull request Aug 17, 2017
Sean Kelly added 16 commits September 5, 2017 15:48
Save the Travis configuration so the builders can get options that are
to be added. The defaults matrix will have the option to be overriden
but it needs to be overriden somewhere and that needs to be available
when triggering jobs.
TravisYml needs to have the configuration so it can override its
defaults when parsing the .travis.yml file.
The default matrix lists all of the options for the thing that is being
built. The default os and dist match what Travis-CI does, with os set to
"linux" and dist set to "precise".

The default matrix also sets the keys to search for in the .travis.yml
file to expand the build matrix. Each language has different keys about
what versions to use. Many use only one, the name of the language, but
others have a differently named key or have multiple keys. Support the
common, simple case by allowing a language to point to a list or tuple
to list the versions to run by default.
Expand the overall matrix to build with the given os and dist options
from the config.
Now everything testing the matrix needs the os and dist keys set.
@seankelly
Copy link
Member Author

Rebased to fix conflicts.

@perlun
Copy link
Contributor

perlun commented Nov 8, 2017

@tardyp What do you think of this, does it need further updates to be mergeable?

@levitte
Copy link
Contributor

levitte commented Dec 15, 2019

I like a lot of what I see here and am prepared to give is a try or even help out.

Regarding os vs dist, If you look back at Travis, both are supported there, and it will simply ignore combinations that just don't fit together (i.e. there's presumably no Travis worker that supports that combination). I don't see an issue with that.

Another thing that's worth adding, which is also supported on Travis since not too long, is arch.

Looking back at #32, I think @vdsbenoit's idea to add the possibility to specify working combinations for the workers, no matter what form (I think that the possibility is sorely lacking for "native" Workers (i.e. not DockerWorker or HyperWorker)). Yes, I understand that there is a way to specify an image using DockerWorker, but doesn't that assume that all workers are able to run all desired images? That's hardly workable with multiple worker platforms, for example...

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

Successfully merging this pull request may close these issues.

None yet

5 participants