"snapcraft validate" and "snapcraft gated" commands #813

ralsina · Sep 19, 2016

Implements snapcraft validate and gated commands, according to discussions and specs.

elopio · Sep 20, 2016

elopio reviewed Sep 20, 2016

View changes

hey, that's cheating! :)

Sorry, that was just temporary while I figure out how to make main have less than a 12 complexity with 12 commands, adding a XXX comment :-)

Done by adding a specific flake8 comment to disable this check in the function that was failing.

Rationale is: I can make the complexity "go away" by doing things like a dictionary dispatch but IRL it's replacing a straightforward if/elif chain with something much less obvious.

elopio · Sep 20, 2016

elopio reviewed Sep 20, 2016

View changes

I would call this: print_gated_snaps.

or show_gated

I was going with the convention of functions being called the same as the command. If you guys like that better, no problem for me.

fair enough from me.

I like having the functions match the command.

sergiusens · Sep 21, 2016

sergiusens reviewed Sep 21, 2016

View changes

is there a scenario where another zero value would make if not search_results not a desirable way to write this?

No such scenario. I sorta like this better since None is what the called function returns, but am ok with changing it :-)

let's change it please. Thanks

Changed

if key: reads better here, is there a possibility for it to be a zero value?

There is a remote chance of someone doing something like --key='' I guess but that ends up being None. Changing it, since empty strings are invalid key names anyway.

what would this error look like?

Honestly, not very good, like

Expecting value: line 1 column 1 (char 0)

I'd change it to a generic "Invalid response from server" maybe?

Yes please, the json decode error is just not friendly for a user.

What would help with debugging this later is to add a logger.debug line with response somewhere in the message. Like logger.debug('Invalid response from the server when doing blah: {} {}'.format(response.code, response.text))

Done.

cprov · Sep 23, 2016

cprov reviewed Sep 23, 2016

View changes

@ralsina let's drop this search-based query by the new snap-details endpoint caller:

store.cpi.get_package(self, snap_name, channel, arch):

https://github.com/snapcore/snapcraft/blob/master/snapcraft/storeapi/__init__.py#L291

ack, replacing.

Done.

cprov · Sep 23, 2016

cprov reviewed Sep 23, 2016

View changes

Roberto, expected_validations has to be optional as well, otherwise you end up passing a and expected result that is completely irrelevant in the testing scenario.

Done

cprov · Sep 23, 2016

cprov reviewed Sep 23, 2016

View changes

Like this call, the expected_validations is not relevant here, only the expected_error

Fixed

cprov · Sep 23, 2016

cprov reviewed Sep 23, 2016

View changes

I think snapcraft guidelines enforce having individual integration test files for each command. Keeping it this way is simpler than arguing for a guidelines change (small boilerplate)

I'll split it.

Done

cprov · Sep 23, 2016

cprov reviewed Sep 23, 2016

View changes

get_package() is not prepared for receiving arch=None, it's pushing a bogus header for CPI waiting for a disaster to happen ... arch is not important for snap-id lookups, so adjust get_package to suppress 'X-Ubuntu-Architecture' is arch is None (similar tweak on errors.SnapNotFound)
Also, I don't see much value in this wrapper if SnapNotFound message is sane, no need to turn it into RuntimeError, 2 different things.

Done. I also fixed a similar wrapper in _store.download

cprov · Sep 23, 2016

cprov reviewed Sep 23, 2016

View changes

"developer_id" will be the account-id of the uploader of the latest revision available in stable channel, because of package-collaborating (sharing) it might not be the "publisher", there is no "publisher_id" in CPI. More importantly, it's not the currently authenticated "account-id", so I think it will be less ambiguous (little more expensive) if you simply fetch account-info.account-id.

Ack, will do.

cprov · Sep 23, 2016

cprov reviewed Sep 23, 2016

View changes

as commented above, "developer_id" is not what you are looking for for validation assertions, it's always signed by the publisher.

Done.

So, you don't need 'developer_id' and 'release' in the current code.

Fixed.

sergiusens · Sep 27, 2016

sergiusens reviewed Sep 27, 2016

View changes

Thanks, looks generally good, but for a better review I need to sit down and read the design docs for validate and gated. Will get back to you again

Is there a reason for this method?

Just that it was common code between gated and validate. It used to be more complicated, it is now too trivial not to inline, will change it.

Mind using !r here?

Sure!

Same !r

Sure! For all args?

cprov · Sep 27, 2016

cprov suggested changes Sep 27, 2016

View changes

Looks good, few points to be improved inline.

Could you, please, squash the ~50 commits to make the PR cleaner.

The check below seems to be more effective and the pattern used in other places.

if os.getenv('TEST_STORE', 'fake') != 'fake':

Done.

this indentation is a little odd ... What about the following:

    validations = [('snap-1', '3'), ('snap-2', '5')]
    exit_code = self.gated('ubuntu-code', validations)
    self.assertEqual(0, exit_code)

(repeated in several tests below)

this code is gone because SnapNotFoundError will be raised to top-level exception handler with equivalent info, right ?

Exactly.

I don't understand what this function buys us. Can't callsites call store.cpi.get_package() directly ?
Also, you've made "arch" optional, no need to pass None.

Well, it was more complicated before :-) I agree it has become a bit too simple and can just be inlined.

as commented above, "developer_id" is not what you are looking for for validation assertions, it's always signed by the publisher.

Done.

So, you don't need 'developer_id' and 'release' in the current code.

Fixed.

this is hardly a debug message, it's an error. What about making sure the raised exception has enough information to guide the user and that's it.
If our servers are not returning proper JSON and we don't know already ... it's more like our problem.

This error usually will mean something like "we are getting HTML instead of JSON because we are pointing to the wrong URL" and the error message will say "Bad response". I think this was suggested by @sergiusens in the previous review, happy to change it if he's ok with it.

ok, shows additional data if '-d' is given, I am sold.

@cprov I need this for the odd case of having issues in production where I can just tell people to add --debug :-)

mitigated by https://code.launchpad.net/~cprov/software-center-agent/sign-build-errors/+merge/306939

Awesome. Want me to remove the XXX?

Only if it fixes your problem.

would textwrap.dedent() help you here ?

It will look nicer. Changing it.

elopio · Sep 27, 2016

elopio reviewed Sep 27, 2016

View changes

There's something weird with your diff, this shouldn't be from your PR.

I did something weird trying to collapse the commits I guess. I'd revert this one, but it's the same as master, right? I have no idea what to do about it :-)

elopio · Sep 27, 2016

elopio reviewed Sep 27, 2016

View changes

So, we will never be able to test this against a real system? There is no way to seed some valid values there?

The main problem is that you need the logged in user to match against the publisher of several snaps in the store. I suppose we can make it work by creating a user account, uploading snaps for it and sharing the password between testers? (and also making the name of those snaps match the ones used internally on the tests... one is 'ubuntu-core' at this point)

Yes, that sounds "good" to me. We have u1test+snapcraft@canonical.com, I can give you the password.

elopio · Sep 27, 2016

elopio reviewed Sep 27, 2016

View changes

This function is too long, can you split it? You are splitting it with comments, it seems it could be nice to have one smaller function per comment.

Sure! I'll do it tomorrow morning.

I split it in 3, there is a largish chunk in the loop that's hard to split without writing functions with many args, which are a separate readability problem. Let me know if you like the current split or want me to split finer.

It looks better now, thanks.

elopio · Sep 27, 2016

elopio reviewed Sep 27, 2016

View changes

#noqa here wouldn't hurt, it could be nicer to read.

Funny part: the NOQA comment goes 80 lines below here :-D

elopio · Sep 27, 2016

elopio reviewed Sep 27, 2016

View changes

well, actually ;)
I think it could be nice to split the commands that handle keys from the commands that handle snap uploads and releases. Not a big deal though, just if you agree.

elopio · Sep 27, 2016

Thanks Roberto, this looks pretty good. I've left some questions, I'm just worried about the lack of integration tests against the real server.
And please don't forget to leave your comments and suggestions :) If there's something you hated while implementing these features, let us know.

ralsina · Sep 27, 2016

Not hated, but docopt is weird once you have these many commands (I know sergio is removing it)

sergiusens · Sep 28, 2016

El martes, 27 de septiembre de 2016 20h'19:10 ART, Roberto Alsina
notifications@github.com escribió:

Not hated, but docopt is weird once you have these many
commands (I know sergio is removing it)

I am sort of settled on click. Not sure if due to everything I read or the
irony 🙃

Enviado con Dekko desde mi dispositivo Ubuntu

ralsina · Sep 28, 2016

Ok, I think I covered all suggestions by @cprov and @elopio, so waiting for final reviews. Thanks for the effort reviewing this guys :-D

cprov · Sep 28, 2016

cprov suggested changes Sep 28, 2016

View changes

Thanks Roberto,

Few points to address and also make sure tests are passing.

I would use os.getenv('TEST_STORE', 'fake') just to be on the safe-side in case TEST_STORE is not defined.

getenv returns None for undefined vars...

Doh. Of course someone may run this manually :-)

done

ditto.

done

I think we should resolve the snap-id via account-info (logged users accessible snaps) instead of hitting CPI and require an active publication in 'stable'.

Also 'None' is not needed, arch is optional

Argh, forgot to remove it when I made it optional :-P
I'll look into resolving via account-info now.

Done

Again, snap-id comes from account-info and 'None' is obsolete.

Done

Right, for the validated snaps (3rd-part most likely) we hit CPI and require stable publications __ for now, because I wonder if we would even set things up while all snaps are in unstable channels, but I am sure we can sort this out later.
None is not needed.

This only affects automatic updates, right? do we even do those for other channels?

You are right, the happy path is enable refresh-control, no validation nothing gets updated even if the controlled snaps reach 'stable'. Once there one overrides refresh-control and installs the not-yet-validation snaps, run tests, then create the corresponding validations.
Thanks for clarifying this.

This function is too long, can you split it? You are splitting it with comments, it seems it could be nice to have one smaller function per comment.

Sure! I'll do it tomorrow morning.

I split it in 3, there is a largish chunk in the loop that's hard to split without writing functions with many args, which are a separate readability problem. Let me know if you like the current split or want me to split finer.

It looks better now, thanks.

this is hardly a debug message, it's an error. What about making sure the raised exception has enough information to guide the user and that's it.
If our servers are not returning proper JSON and we don't know already ... it's more like our problem.

This error usually will mean something like "we are getting HTML instead of JSON because we are pointing to the wrong URL" and the error message will say "Bad response". I think this was suggested by @sergiusens in the previous review, happy to change it if he's ok with it.

ok, shows additional data if '-d' is given, I am sold.

@cprov I need this for the odd case of having issues in production where I can just tell people to add --debug :-)

mitigated by https://code.launchpad.net/~cprov/software-center-agent/sign-build-errors/+merge/306939

Awesome. Want me to remove the XXX?

Only if it fixes your problem.

josepht · Sep 29, 2016

josepht reviewed Sep 29, 2016

View changes

LGTM

sergiusens · Sep 29, 2016

sergiusens requested changes Sep 29, 2016

View changes

i think I have the last set of feedback from your Initial skeleton for validate 😉

Making [<snap-revision> ...] [--key-name=<key-name>] optional allows me to run
snapcraft validate <snap-name> which provides no useful output and probably invalid:

$ snapcraft validate telegram-sergiusens
Getting details for telegram-sergiusens
$

<snap-revision> is f the form <snap-name>=<revision>? This needs a better arg-name.

maybe <snap-name-revision-pair>

Better arg-name ... maybe "validation"?

Yes, it's always snap-name=revision

And it's most definitely not optional, there must be at least one. How does one tell docopt "one or more of these"?

Yes, calling "snapcraft validate telegram-sergiusens" currently does nothing. I can add a check for "at least one validation" if there's no way to tell docopt to do it.

@sergiusens I just pushed a change where it now is <validation>... which renames it, and makes it required at least once.

http://try.docopt.org/?doc=snapcraft%0D%0A%0D%0AUsage%3A%0D%0A++snapcraft+validate+%3Csnap-name%3E+%28--keys+%3Cone%3E+%7C+%3Ctwo%3E%29&argv=validate+snap+--keys+1

this is hardly a debug message, it's an error. What about making sure the raised exception has enough information to guide the user and that's it.
If our servers are not returning proper JSON and we don't know already ... it's more like our problem.

This error usually will mean something like "we are getting HTML instead of JSON because we are pointing to the wrong URL" and the error message will say "Bad response". I think this was suggested by @sergiusens in the previous review, happy to change it if he's ok with it.

ok, shows additional data if '-d' is given, I am sold.

@cprov I need this for the odd case of having issues in production where I can just tell people to add --debug :-)

sergiusens · Sep 30, 2016

ok to test

initial skeleton of validate command

38cab15

ralsina changed the title from [WIP] "snapcraft validate" and "snapcraft gated" commands to "snapcraft validate" and "snapcraft gated" commands Sep 27, 2016

Validate and gated commands implementation

fd426eb

solved conflicts

7bf2796

formatting of the help string

71039ed

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

"snapcraft validate" and "snapcraft gated" commands (#813)
LP: #1629335

6c44f23

sergiusens referenced this pull request Sep 20, 2017
Merged
store: handle revoked developers #1554
6 of 6 tasks complete

@@ -49,7 +49,7 @@ python3 -m coverage && coverage="true"
          run_static_tests(){
              SRC_PATHS="bin snapcraft integration_tests snaps_tests"
-        -    python3 -m flake8 --max-complexity=10 $SRC_PATHS
+        +    python3 -m flake8 --max-complexity=12 $SRC_PATHS

+        +        try:
+        +            response_json = response.json()
+        +        except (AttributeError, JSONDecodeError):
+        +            raise errors.StoreValidationError(snap_id, response)

+        +    store = storeapi.StoreClient()
+        +    # Get data for the gating snap
+        +    with _requires_login():
+        +        snap_data = _get_snap_data(snap_name, store)

@@ -186,3 +186,27 @@ def update_name_and_version(self, project_dir, name=None, version=None):
                      else:
                          print(line)
                  return updated_project_dir
+        +
+        +    def gated(self, snap_name, expected_validations, expected_error=None):

+        +        self.assertEqual(1, self.gated('ubuntu-core', [
+        +            ('snap-1', '3'),
+        +            ('snap-2', '5'),
+        +        ], expected_error='Have you run "snapcraft login'))

snapcore/snapcraft

"snapcraft validate" and "snapcraft gated" commands #813

ralsina commented Sep 19, 2016 •

Edited 1 time

ralsina
Sep 27, 2016

elopio reviewed Sep 20, 2016

View changes

elopio reviewed Sep 20, 2016

View changes

sergiusens reviewed Sep 21, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

ralsina changed the title from [WIP] "snapcraft validate" and "snapcraft gated" commands to "snapcraft validate" and "snapcraft gated" commands Sep 27, 2016

sergiusens reviewed Sep 27, 2016

View changes

cprov suggested changes Sep 27, 2016

View changes

elopio reviewed Sep 27, 2016

View changes

elopio reviewed Sep 27, 2016

View changes

elopio reviewed Sep 27, 2016

View changes

elopio reviewed Sep 27, 2016

View changes

elopio reviewed Sep 27, 2016

View changes

elopio commented Sep 27, 2016

ralsina commented Sep 27, 2016

sergiusens commented Sep 28, 2016

ralsina added some commits Sep 28, 2016

ralsina commented Sep 28, 2016

cprov suggested changes Sep 28, 2016

View changes

ralsina added some commits Sep 28, 2016

josepht reviewed Sep 29, 2016

View changes

sergiusens requested changes Sep 29, 2016

View changes

sergiusens commented Sep 30, 2016

sergiusens merged commit `2d429cf` into snapcore:master Sep 30, 2016
4 of 6 checks passed

4 of 6 checks passed

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

sergiusens referenced this pull request Sep 20, 2017

store: handle revoked developers #1554

		@@ -0,0 +1,80 @@
		+# -- Mode:Python; indent-tabs-mode:nil; tab-width:4 --

snapcore/snapcraft

Join GitHub today

"snapcraft validate" and "snapcraft gated" commands #813

Conversation

ralsina commented Sep 19, 2016 • edited Edited 1 time ralsina Sep 27, 2016

elopio reviewed Sep 20, 2016 View changes

elopio reviewed Sep 20, 2016 View changes

sergiusens reviewed Sep 21, 2016 View changes

cprov reviewed Sep 23, 2016 View changes

cprov reviewed Sep 23, 2016 View changes

cprov reviewed Sep 23, 2016 View changes

cprov reviewed Sep 23, 2016 View changes

cprov reviewed Sep 23, 2016 View changes

ralsina Sep 23, 2016 • edited Edited 1 time ralsina Sep 23, 2016

cprov reviewed Sep 23, 2016 View changes

cprov reviewed Sep 23, 2016 View changes

ralsina changed the title from [WIP] "snapcraft validate" and "snapcraft gated" commands to "snapcraft validate" and "snapcraft gated" commands Sep 27, 2016

sergiusens reviewed Sep 27, 2016 View changes

ralsina Sep 27, 2016 • edited Edited 1 time ralsina Sep 27, 2016

cprov suggested changes Sep 27, 2016 View changes

cprov Sep 27, 2016 • edited Edited 1 time cprov Sep 27, 2016

elopio reviewed Sep 27, 2016 View changes

ralsina Sep 27, 2016 • edited Edited 1 time ralsina Sep 27, 2016

elopio reviewed Sep 27, 2016 View changes

ralsina Sep 27, 2016 • edited Edited 1 time ralsina Sep 27, 2016

elopio reviewed Sep 27, 2016 View changes

elopio reviewed Sep 27, 2016 View changes

elopio reviewed Sep 27, 2016 View changes

elopio commented Sep 27, 2016

ralsina commented Sep 27, 2016

sergiusens commented Sep 28, 2016

ralsina added some commits Sep 28, 2016

Some checks were not successful

Some checks were not successful

ralsina commented Sep 28, 2016

cprov suggested changes Sep 28, 2016 View changes

ralsina added some commits Sep 28, 2016

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

josepht reviewed Sep 29, 2016 View changes

All checks have passed

sergiusens requested changes Sep 29, 2016 View changes

ralsina Sep 29, 2016 • edited Edited 1 time ralsina Sep 29, 2016

ralsina commented Sep 19, 2016 •

Edited 1 time

ralsina
Sep 27, 2016

elopio reviewed Sep 20, 2016

View changes

elopio reviewed Sep 20, 2016

View changes

sergiusens reviewed Sep 21, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

ralsina Sep 23, 2016 •

Edited 1 time

ralsina
Sep 23, 2016

cprov reviewed Sep 23, 2016

View changes

cprov reviewed Sep 23, 2016

View changes

sergiusens reviewed Sep 27, 2016

View changes

ralsina Sep 27, 2016 •

Edited 1 time

ralsina
Sep 27, 2016

cprov suggested changes Sep 27, 2016

View changes

cprov Sep 27, 2016 •

Edited 1 time

cprov
Sep 27, 2016

elopio reviewed Sep 27, 2016

View changes

ralsina Sep 27, 2016 •

Edited 1 time

ralsina
Sep 27, 2016

elopio reviewed Sep 27, 2016

View changes

ralsina Sep 27, 2016 •

Edited 1 time

ralsina
Sep 27, 2016

elopio reviewed Sep 27, 2016

View changes

elopio reviewed Sep 27, 2016

View changes

elopio reviewed Sep 27, 2016

View changes

cprov suggested changes Sep 28, 2016

View changes

josepht reviewed Sep 29, 2016

View changes

sergiusens requested changes Sep 29, 2016

View changes

ralsina Sep 29, 2016 •

Edited 1 time

ralsina
Sep 29, 2016