improve travis-ci defaults to leverage build stages

[rtib]

Hi there, I'd propose some changes to travis-ci defaults. Main goal is to leverage build stages by default. Along with some maintenance and bug fixing the main changes in this PR are:

• (improvement) define stages: static, spec, beaker, deploy
• (feature) deploy stage is limited to version tag
• (maintenance) remove unnecessary global env
• (bugfix) add bundler_args "--with system_tests" to docker_defaults
• (improvement) split static code analysis jobs to multiple jobs on static stage
• (improvement) move spec test jobs to spec stage
• (maintenance) replace default spec job with explicit Puppet 6 job
• (maintenance) order spec test jobs by major version
• (improvement) introduce deploy job on deploy stage

I've been using such a setup for some time now and would like to push them upstream. As an working example, have a look on

• my puppet-geoip module
• and its Travis-CI Build

[scotje]

Hi @rtib !

These changes look solid to me. Given their wide impact I think we should wait for a bit and let other community members weigh in as well.

Thanks for your contribution!

[bmjen]

/cc @cdenneen @dhollinger @bastelfreak @rnelson0

This PR touches a lot of parts of the base pdk-templates, please take a look and see if any of these changes will impact you negatively.

[bastelfreak]

Hi everybody.
thanks for pinging, @bmjen. I don't think that this effects Vox Pupuli modules, we currently don't use pdk and still rely on modulesync. It might be a good idea to adapt this. Did you test if this as any speed impacts? Puppet and Vox Pupuli have more than the usual two concurrent travis jobs. Does this change make the average run faster or slower?

[rtib]

Hi @bastelfreak. It's hard to predict the impact on average runtime. The idea behind this is to order the tasks and use the results of cheaper tests to predict the results of more resource and time consuming tests.

Having the cheap static analysis jobs e.g. syntax, lint and rubocop run first. On the next stage more expensive spec tests will run only if the first stage was completed and fine. The most expensive beaker tests will start only if all spec tests were successful. Finally, if the commit has a version tag, the deploy job is carried out.

This makes the average runtime of a fully successful CI build longer. But if you have some syntax, lint or rubocop offences, the process will stop very early and does not allocate resources for more expensive jobs. Most of all, if any of your spec tests fail, then you don't need to waste time and resources on beaker.

[npwalker]

Almost all of the time spent on the static code analysis steps comes from spinning up the container and installing dependencies. This is why I moved them all into one step in #88.

All of those should stay as one step to reduce waste of resources.

How does the DEPLOY_TO_FORGE setting work? Are we suddenly opting everyone into something by putting that there?

[rtib]

Hi @npwalker ,

it's a trade-off between reducing CI resources, the time producing feedback and grouping the checks. Having many checks in one step makes it hard to see the cause if a single one was failed. I found it more valuable to have distinct jobs for git sanity, puppet code validation and rubocop. Indeed, this consumes more resources at travis, but these jobs are still running in parallel, therefore the time impact should not be as high.

The deployment logic works as follows (see details on the generated travis.yml in the above example):

1. the deploy stage exists as last stage only if the build has a version tag (see rtib/puppet-geoip/.travis.yml#L21)

2. if all stages succeeded, the deploy stage will run and set DEPLOY_TO_FORGE=1 (.travis.yml#L57)

3. deployment will happen only if the condition is met (.travis.yml#L73).

That makes a puppet module to be tested and deployed to forge automatically if you push a version tag. Note, that user and secret also needs to be set otherwise the deploy job will fail.

[ghoneycutt]

I really like this approach of stages because it makes it really easy to understand what is failing the tests from a glance.

I don't recommend we use this functionality as is though because it adds a lot of extra time to the tests. The test at https://travis-ci.org/rtib/puppet-geoip/jobs/470712805 runs for 2m11s with 20s running the actual tests and the other time setting up the testing environment. This would greatly impact the time to test by adding more environments.

Suggest that we combine the static and spec tests into one stage which would alleviate the issue above. This also fits better with how people test now, which is to runs static and spec tests for each puppet version, run a bunch of beaker tests and optionally run a deploy.

[dhollinger]

I'm with @ghoneycutt - having multiple stages and multiple "jobs" in those stages allows a developer to get quick results on specific types of tests instead of having to dig through the test logs to find what failed. If rubocop fails, then all I have to do is look at the bottom of the rubocop log. Similar process for syntax, linting, and metadata checks. If they are all pulled together into one, it can occasionally become difficult to find the log that tells you what and where something failed.

The separate stages fit with the "Fail fast and Early" mantra.

[npwalker]

I like the PR as it stands now because it adds in build stages which is an improvement and we don't have to hash out whether the checks should be split apart or not. In order to get this change in I think we should agree to limit the scope to just adding build stages.

It looks like the PR needs to be rebased then I suspect it's good to go.

If we want to discuss breaking apart the static checks it should be moved to another ticket or PR. Although I'm still of the opinion that we should not break them apart as to be responsible users of Travis' free offering.

[scotje]

Sounds like this just needs a rebase at this point?

Thanks @bastelfreak @npwalker @ghoneycutt @dhollinger for weighing in and helping this get to a good compromise!

And thank you to @rtib for diligently answering questions and integrating everyone's feedback!

[rtib]

Have rebased it.

Additionally, I renamed the "beaker" stage to "acceptance" which better fit the purpose rather than the name of the tool currently used.

concerns addressed