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

integrations: use the snapcore/snapcraft docker image in travis #1331

Merged

Conversation

filibtester
Copy link
Contributor

Thanks for helping us make a better Snapcraft!

Checklist:

LP: #BUG_NUMBER

@come-maiz
Copy link
Contributor

@filibtester thanks for the fix!
But you haven't signed the contributors agreement. Take a look at the first point of the checklist up there ^

@@ -210,7 +210,6 @@ def test_enable_successfully(
"Enabling Travis testbeds to push and release 'foo' snaps "
"to edge channel in series '16'",
'Acquiring specific authorization information ...',
'Login successful.',
Copy link
Contributor

Choose a reason for hiding this comment

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

For other reviewers, this change is interesting. Take a look that @filibtester moved the file, because it wasn't being run by ./runtests.sh unit.
So this test got stale, and started failing without us noticing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@filibtester nice catch! Thank you.

@come-maiz
Copy link
Contributor

This is missing one more +1. @kalikiana or @kyrofa, please take a look.

Copy link
Contributor

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

This is missing one more +1. @kalikiana or @kyrofa, please take a look.

Haha, as far as I can see, it doesn't have a +1 from you either! This looks good to me, but I'd appreciate a quick look from @cprov as well.

Copy link
Contributor

@come-maiz come-maiz left a comment

Choose a reason for hiding this comment

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

Sorry, I forgot my approval.

@cprov
Copy link
Contributor

cprov commented Jun 16, 2017

The change looks good and will certainly bump usage of the snapcraft docker image.

Just a small remark, this change doesn't offer any alternative for updating existing integrations, snapcraft enable-ci travis --refresh will continue to only update store credentials. The .travis.yml changes are simple enough this time and probably don't justify investing time for assisted updates.

@come-maiz
Copy link
Contributor

@cprov That's a good point, about the existing integrations out there. However, keeping the old image is safe, as it takes care of upgrading snapcraft and will result in the same snap. And the work required to migrate those old projects would be complex, not required IMO. I'm going to merge this one.

@come-maiz come-maiz merged commit 6d777f7 into canonical:master Jun 20, 2017
@kyrofa kyrofa added this to the 2.32 milestone Jun 23, 2017
kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Aug 3, 2017
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