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

Use deployment primitives from travis #843

Merged
merged 1 commit into from
Jan 24, 2017
Merged

Use deployment primitives from travis #843

merged 1 commit into from
Jan 24, 2017

Conversation

guilhem
Copy link
Contributor

@guilhem guilhem commented Nov 14, 2016

Need advice on this.
I may go further to improve building

@errm
Copy link
Contributor

errm commented Nov 14, 2016

This looks good to me

ping @containous/traefik

@errm
Copy link
Contributor

errm commented Nov 16, 2016

Needs a rebase

@guilhem
Copy link
Contributor Author

guilhem commented Nov 17, 2016

@errm if you are talking about multi-commit, maintainers can merge by squashing commits ;)

@errm
Copy link
Contributor

errm commented Nov 17, 2016

No just the branch was stale...

@vdemeester vdemeester self-assigned this Nov 23, 2016
@emilevauge
Copy link
Member

Ping @containous/traefik, we need a review on this ;)

@vdemeester
Copy link
Contributor

@guilhem needs a rebase. How could we test that though ?

@emilevauge
Copy link
Member

@guilhem are you sure that the behavior is the same in the case of a pull request (deploy-pr.sh removed) ?

@errm
Copy link
Contributor

errm commented Dec 8, 2016

Yeah looks like the behaviour from deploy-pr.sh is not replicated, can we add that back in please @guilhem ?

@guilhem
Copy link
Contributor Author

guilhem commented Dec 8, 2016

@errm @emilevauge Yes, I didn't replicated deploy-pr.sh because it's just simply not working :)

https://travis-ci.org/containous/traefik/jobs/181516876#L1946
https://travis-ci.org/containous/traefik/jobs/181516879#L1949

@emilevauge
Copy link
Member

@guilhem
Copy link
Contributor Author

guilhem commented Dec 16, 2016

humm...
So I have to fix it ^^

@guilhem
Copy link
Contributor Author

guilhem commented Dec 16, 2016

@emilevauge I think I've found the problem.
Only PR done by you (or people with write access) have this feature working because of this: https://docs.travis-ci.com/user/pull-requests#Pull-Requests-and-Security-Restrictions

@emilevauge
Copy link
Member

@guilhem
Copy link
Contributor Author

guilhem commented Dec 16, 2016

@emilevauge but https://travis-ci.org/containous/traefik/jobs/182320106#L2085 fail :)

Travis Job you point was after merging so inside a "secure" branch (master)

@guilhem
Copy link
Contributor Author

guilhem commented Dec 16, 2016

But maybe we want to have a rolling docker for each commit pushed inside "master"

@emilevauge
Copy link
Member

Yes, check the merge build, not the pull request build: https://travis-ci.org/containous/traefik/jobs/182236245#L2087

@guilhem
Copy link
Contributor Author

guilhem commented Dec 16, 2016

@emilevauge should be ok. Plus:

  • speed improved
  • right docker version tested

@emilevauge
Copy link
Member

@guilhem I don't think creating a Docker image by commit is a good idea. Usually, we need to atomically take the whole pull request to get it working. Could you rollback to the old behavior?

@guilhem
Copy link
Contributor Author

guilhem commented Dec 20, 2016

@emilevauge docker image is pushed not by commit, but with "latest" commit (the merged if there is one).

@emilevauge
Copy link
Member

Oops, you right ;)

@guilhem
Copy link
Contributor Author

guilhem commented Dec 23, 2016

gentleping @containous/traefik

Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐸
@guilhem could you rebase and squash your commits 👼

@guilhem
Copy link
Contributor Author

guilhem commented Jan 5, 2017

@vdemeester done :)

@emilevauge
Copy link
Member

@guilhem do you know why tests are failing ?

@guilhem
Copy link
Contributor Author

guilhem commented Jan 6, 2017

@emilevauge no idea. can you try to re-run failing tests?

@emilevauge
Copy link
Member

@guilhem I relaunch the tests 2 times and still the same result :'(

@guilhem
Copy link
Contributor Author

guilhem commented Jan 9, 2017

@emilevauge I think bug come from github.com/libkermit/compose or github.com/docker/libcompose or association of it.
Test for docker 1.10 are ok but compose is failing with docker 1.9
It's quite hard to update dependencies between all this projects as it's used by traefik and traefik/integration and they are different packages.

Solutions can be:

  • remove docker 1.9 support (it's not really tested with current setup).
  • To include integrations test in same sources and use flag matching

@guilhem
Copy link
Contributor Author

guilhem commented Jan 9, 2017

it works with docker 1.10 and 1.12

@emilevauge
Copy link
Member

@guilhem we agree with @vdemeester to remove Docker 1.9 support :)

@guilhem
Copy link
Contributor Author

guilhem commented Jan 18, 2017

@emilevauge I'm in holiday right now, but I will do this for Monday before meetup ;)

fast_finish: true
include:
- env: DOCKER_VERSION=1.10.3
- env: DOCKER_VERSION=1.12.5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be 1.12.6 👼
We could also add 1.13.0 but let's do that in a follow-up 👼

@guilhem
Copy link
Contributor Author

guilhem commented Jan 23, 2017

@vdemeester rebased, squashed, fixed etc
:)

@guilhem
Copy link
Contributor Author

guilhem commented Jan 23, 2017

FAIL: basic_test.go:76: SimpleSuite.TestPrintHelp
basic_test.go:89:
    c.Assert(string(output), checker.Contains, "Usage:")
... obtained string = ""
... substring string = "Usage:"

only on 1 environment that was working before :(

@emilevauge
Copy link
Member

@vdemeester, any idea on what's going on here ?

@vdemeester
Copy link
Contributor

Hum I don't see in the failed build but it might be a weirdiness with how the test is written — if the run is particularly slow it might fail 😓 . I'll take a look.

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG it's green 🙄

OSS is 10% inspiration, 90% restarting failed CI builds until they randomly work

— Nolan Lawson https://twitter.com/nolanlawson/status/802309792349855744

LGTM
Thanks a lot @guilhem 👍

@emilevauge emilevauge merged commit fad3038 into traefik:master Jan 24, 2017
@emilevauge emilevauge mentioned this pull request Jan 25, 2017
@ldez ldez added this to the 1.2 milestone Oct 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants