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

git demo snap update - Bug 1595229 #593

Closed
wants to merge 7 commits into from

Conversation

stub42
Copy link

@stub42 stub42 commented Jun 22, 2016

Addresses https://bugs.launchpad.net/snapcraft/+bug/1595229

Adds the 'home' plug to the snap and documentation on how to use it.

@snappy-m-o
Copy link

Can one of the admins verify this patch?

1 similar comment
@snappy-m-o
Copy link

Can one of the admins verify this patch?

@@ -14,3 +14,4 @@ dist
htmlcov
__pycache__
docs/**.html
*~
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you :)

@come-maiz
Copy link
Contributor

I like this. I would like to get @didrocks to look at it, because maybe in his plans for the tour and the website there's an explanation of the home interface. In that case we could get rid of this demo, and move everything you wrote to the tour. If it's not in his short-term plans, lets land this one.

And this is missing a test. Take a look at the ones in snaps_tests/demos_tests and see how we can run the connect command, and then verify that the snap is working. Let me know if you need a hand with this.

@snappy-m-o
Copy link

Can one of the admins verify this patch?

1 similar comment
@snappy-m-o
Copy link

Can one of the admins verify this patch?

@didrocks
Copy link
Contributor

Agreed with Leo, this demo is becoming redondant once the tour/documentation will talk more about plugs. However, this isn't going to be released short term, so +1 on shipping something like this.

@sergiusens
Copy link
Collaborator

retest this please
ok to test

@snappy-m-o
Copy link

Can one of the admins verify this patch?

1 similar comment
@snappy-m-o
Copy link

Can one of the admins verify this patch?

'git:home', 'ubuntu-core:home'])
self.assertTrue(
self.snappy_testbed.run_command(['/snap/bin/git',
'--version']).startswith('git'))
Copy link
Contributor

@come-maiz come-maiz Jun 24, 2016

Choose a reason for hiding this comment

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

Thanks for adding the test!
But I was thinking that you could do something like:
self.snappy_testbed.run_command(['mkdir', '-p', '$HOME/tmp/foo'])
self.snappy_testbed.run_command(['/snap/bin/git', 'init', '$HOME/tmp/foo'])

This might need some more work on the run helpers. I'm around to help.

Also, now that this is more extensively tested, would you mind removing the scenario in snaps_tests/demos/tests.py?

@stub42
Copy link
Author

stub42 commented Jun 27, 2016

Confirming that the 'home' interface does what it claims does not need to be part of this test, so I'd rather leave it like it is than pollute $HOME with temp directories.

Or is this sandboxed somehow and I don't have to worry about cleaning up $HOME safely and reliably?

I have removed the redundant test in snaps_tests/demos/tests.py

@stub42 stub42 closed this Jun 27, 2016
@stub42 stub42 reopened this Jun 27, 2016
@snappy-m-o
Copy link

Can one of the admins verify this patch?

1 similar comment
@snappy-m-o
Copy link

Can one of the admins verify this patch?

@come-maiz
Copy link
Contributor

The home is not sandboxed. You would have to rm anything you put in there to leave it clean.
We could make it a fixture, to be called by all tests if it becomes ugly.
I am ok with this test checking that the home interface works too. I see this suite as testing the integration with ubuntu and snapd, so checking that if we declare an interface in the snapcraft.yaml, then the snap won't work until we connect that interface is a nice test for me. Of course, there are downsides to this, feel free to disagree :) Also, one can argue that a test called git shouldn't be where we place an interface test, that's why I'm so happy about the tour work, as it documents and tests the features progressively and independently.

I will leave you my 👍 here, as this is clearly way better than before. And I'm waiting to hear your opinion on my comments. We can push many things for later, reporting bugs for them.

@snappy-m-o
Copy link

Can one of the admins verify this patch?

@snappy-m-o
Copy link

Can one of the admins verify this patch?

@stub42
Copy link
Author

stub42 commented Jul 5, 2016

The test seems to be legitimately failing, and I haven't got them running locally yet to debug.

@come-maiz
Copy link
Contributor

Can you please resolve the conflict so we can take a look at the failing test?

@snappy-m-o
Copy link

Can one of the admins verify this patch?

1 similar comment
@snappy-m-o
Copy link

Can one of the admins verify this patch?

@kyrofa
Copy link
Contributor

kyrofa commented Aug 12, 2016

There seems to be little activity here. I'll go ahead and close this for now-- feel free to reopen once it's ready to progress.

@kyrofa kyrofa closed this Aug 12, 2016
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

6 participants