Added 4 more methods #10

me-sosa · 2019-02-13T16:25:34Z

show_tags_per_device: gets all the devices with the specified app_id
show_device_env_vars: Shows overriden ENV VARS in each device
show_app_env_vars: Shows application ENV VARS with default value
4. set_device_env_vars: Override ENV VARS with new values taken from the tags

Tested on schAlpha with two devices and different test cases.

resin_release_tool/releaser.py

Roger · 2019-02-14T08:36:28Z

resin_release_tool/releaser.py

+            all_devices = self.models.device.get_all_by_application_id(app_info['id'])
+            tags_per_device = []
+            for device in all_devices:
+                tags_per_device.append({'uuid':device['uuid'],


maybe you can use something like this, so you don't have that many api calls, https://github.com/mobilityhouse/resin-release-tool/blob/master/resin_release_tool/releaser.py#L18

Roger · 2019-02-14T09:07:27Z

You are setting every tag to env and should be get all the tags of the devices in a dict and set it to RESIN_TAG_LIST, using json.dumps

me-sosa · 2019-02-14T09:18:54Z

@Roger

You are setting every tag to env and should be get all the tags of the devices in a dict and set it to RESIN_TAG_LIST, using json.dumps

So RESIN_TAG_LIST should look like: {'STATUS': 'not commissioned', 'COLOR';'orange', etc} right?
Now my question is, which method will receive this?

munyildiz · 2019-02-14T12:08:43Z

resin_release_tool/releaser.py

@@ -21,6 +23,69 @@ def get_canaries(self):
            if tag['tag_key'] == 'CANARY']
        return canaries

+    def get_tags_per_device(self):


maybe it makes it clearer to have a short docstring here?

…a string containing all the tags. NOTE: During this bug fix we've found that variable names starting with "BALENA_" or "RESIN_" are forbidden.

me-sosa · 2019-02-14T13:01:02Z

g

@Roger please check the latest commit. I already tested it on schAlpha and it works.

resin_release_tool/cli.py

dequis · 2019-02-14T13:53:47Z

resin_release_tool/releaser.py

+
+    def get_app_env_vars(self):        
+        allvars = self.models.environment_variables.application.get_all(int(self.app_id))
+        list_of_app_env_vars = [{'id':var['id'], var['name']:var['value']} for var in allvars]


Any particular reason this format was chosen?

If I'm understanding things right, it's:

[{"id": 1234, "something": "its value"}, {"id": 2345, "foobar": "baz"}]

Which seems odd to me, using var['name'] as dictionary key makes it hard to access stuff programmatically (you'd have to iterate over the dictionary's keys and exclude the one named "id" to access the actual key/value pair)

It would make things simpler to either keep the original format returned by get_all() (since the name is reliably in the dictionary key named 'name') or rearrange it into a dictionary like this:

{"something": {"id": 1234, "value": "its value"}, "foobar": {"id": 2345, "value": "baz"}}

So you can just access dict["something"]["value"] without for loops.

It's been refactored now, thanks for your help!

This function looks the same to me though.

dequis · 2019-02-14T13:54:20Z

resin_release_tool/releaser.py

+        list_of_env_vars_per_device = []
+        for device in self.uuid_list:
+            allvars = self.models.environment_variables.device.get_all(device)
+            list_of_vars = [{'id':var['id'], var['name']:var['value']} for var in allvars]


This seems very similar to the code in get_app_env_vars, consider putting that in a separate function.

dequis · 2019-02-14T13:56:15Z

resin_release_tool/releaser.py

+        for device in self.uuid_list:
+            allvars = self.models.environment_variables.device.get_all(device)
+            list_of_vars = [{'id':var['id'], var['name']:var['value']} for var in allvars]
+            list_of_env_vars_per_device.append({device:list_of_vars})


Pretty much the comment on structure as above: Any particular reason to choose this...

[{"some_device_id": [...list...]}, {"other_device_id": [...list...]}]

over just this?

{"some_device_id": [...list...], "other_device_id": [...list...]}

dequis · 2019-02-14T13:57:28Z

resin_release_tool/releaser.py

+                value = json.dumps(device['tags'])
+                self.models.environment_variables.device.create(device['uuid'], 'DEVICE_TAG_LIST', value)
+                print("creating/overriding var: ", device['uuid'], 'DEVICE_TAG_LIST', value)
+            except:


This "except" is too broad and could hide other problems.

This is something pylint would point out, I think this project is missing some integration there.

Noted, for sure it will require some more refactoring and integrations in the future but for now I think we can merge it since we need to provide the tags to the ops team.
Thanks @dequis @OrangeTux for the review!

Yeah the pylint stuff is out of scope.

But this isn't resolved, the except is still too broad

resin_release_tool/releaser.py

OrangeTux

I've some minor comments. Also look at @dequis his comments.

resin_release_tool/releaser.py

OrangeTux · 2019-02-15T11:05:27Z

resin_release_tool/releaser.py

+                value = json.dumps(device['tags'])
+                self.models.environment_variables.device.create(device['uuid'], 'DEVICE_TAG_LIST', value)
+                print("creating/overriding var: ", device['uuid'], 'DEVICE_TAG_LIST', value)
+            except:


dequis

Some things are still not addressed (that self.uuid_list is still there)

Roger · 2019-02-15T15:51:31Z

please squash before merge! and thanks for the changes :)

…a string containing all the tags. NOTE: During this bug fix we've found that variable names starting with "BALENA_" or "RESIN_" are forbidden. Refactored "get_tags_per_device", now it is cleaner and shorter. Solved empty uuid list and got rid of "self.uuid_list"

…-tool into env_vars_mod Squashed last 5 commits

dequis · 2019-02-15T16:26:01Z

resin_release_tool/releaser.py

+
+    def get_app_env_vars(self):        
+        all_vars = self.models.environment_variables.application.get_all(int(self.app_id))
+        return [{'id':var['id'], var['name']:var['value']} for var in all_vars]


Oh the original line got refactored but #10 (comment) still applies

dequis · 2019-02-15T16:26:39Z

resin_release_tool/releaser.py

+                    if uuid in elem:
+                        list_of_env_vars = elem[uuid]
+                        for var in list_of_env_vars:
+                            if 'DEVICE_TAG_LIST' in var:


Remember to refactor this whole loop after get_app_env_vars returns a plain dict.

sonarcloud · 2022-03-24T09:44:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

Added 4 more comands to cli and the corresponding methods for each one.

d4f3ffc

Tested on schAlpha with two devices and different test cases.

me-sosa requested review from Roger and tiagofranzen February 13, 2019 16:25

Roger reviewed Feb 14, 2019

View reviewed changes

resin_release_tool/releaser.py Outdated Show resolved Hide resolved

Roger reviewed Feb 14, 2019

View reviewed changes

Replaced for loop with a list comprehension.

e692a16

munyildiz reviewed Feb 14, 2019

View reviewed changes

Solved set_device_env_vars command. Now setting the right env var as …

bf6061a

…a string containing all the tags. NOTE: During this bug fix we've found that variable names starting with "BALENA_" or "RESIN_" are forbidden.

dequis reviewed Feb 14, 2019

View reviewed changes

me-sosa added 2 commits February 14, 2019 18:02

Refactored "get_tags_per_device", now it is cleaner and shorter.

094ba1a

Solved empty uuid list

bb502d8

OrangeTux suggested changes Feb 15, 2019

View reviewed changes

Modified methods after reviews

d84527d

OrangeTux previously approved these changes Feb 15, 2019

View reviewed changes

dequis requested changes Feb 15, 2019

View reviewed changes

Got rid of "self.uuid_list"

1c7e0a6

me-sosa dismissed OrangeTux’s stale review via 1c7e0a6 February 15, 2019 15:50

me-sosa added 3 commits February 15, 2019 17:01

Tested on schAlpha with two devices and different test cases.

d590d81

Merge branch 'env_vars_mod' of github.com:mobilityhouse/resin-release…

271b0e1

…-tool into env_vars_mod Squashed last 5 commits

dequis reviewed Feb 15, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added 4 more methods #10

Added 4 more methods #10

me-sosa commented Feb 13, 2019

Roger Feb 14, 2019

Roger commented Feb 14, 2019

me-sosa commented Feb 14, 2019 •

edited

Loading

munyildiz Feb 14, 2019

me-sosa Feb 14, 2019

me-sosa commented Feb 14, 2019

dequis Feb 14, 2019

me-sosa Feb 14, 2019

dequis Feb 15, 2019

dequis Feb 14, 2019

dequis Feb 14, 2019

dequis Feb 14, 2019

OrangeTux Feb 15, 2019

me-sosa Feb 15, 2019

dequis Feb 15, 2019

dequis Feb 15, 2019

OrangeTux left a comment

OrangeTux Feb 15, 2019

dequis left a comment

Roger commented Feb 15, 2019

dequis Feb 15, 2019

dequis Feb 15, 2019

sonarcloud bot commented Mar 24, 2022

Added 4 more methods #10

Are you sure you want to change the base?

Added 4 more methods #10

Conversation

me-sosa commented Feb 13, 2019

Choose a reason for hiding this comment

Roger commented Feb 14, 2019

me-sosa commented Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

me-sosa commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrangeTux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dequis left a comment

Choose a reason for hiding this comment

Roger commented Feb 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 24, 2022

me-sosa commented Feb 14, 2019 •

edited

Loading