-
Notifications
You must be signed in to change notification settings - Fork 276
Add appengine services and versions to Inventory. #646
Conversation
@@ -30,6 +30,7 @@ | |||
`default_bucket` varchar(255) DEFAULT NULL, | |||
`iap` json DEFAULT NULL, | |||
`gcr_domain` varchar(255) DEFAULT NULL, | |||
`services` json DEFAULT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to see an APPENGINE_SERVICES, APPENGINE_VERSIONS and APPENGINE_INSTANCES tables added, with just json columns for the main data, and the required keys:
CREATE_APPENGINE_SERVICES_TABLE = """
CREATE TABLE {0}
(
id
bigint(20) unsigned NOT NULL AUTO_INCREMENT,
project_id
varchar(255) DEFAULT NULL,
app_id
varchar(255) DEFAULT NULL,
service_id
varchar(255) DEFAULT NULL,
service
json DEFAULT NULL,
PRIMARY KEY (id
),
UNIQUE KEY service_key
(app_id
, service_id
)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
"""
CREATE_APPENGINE_VERSIONS_TABLE = """
CREATE TABLE {0}
(
id
bigint(20) unsigned NOT NULL AUTO_INCREMENT,
project_id
varchar(255) DEFAULT NULL,
app_id
varchar(255) DEFAULT NULL,
service_id
varchar(255) DEFAULT NULL,
version_id
varchar(255) DEFAULT NULL,
version
json DEFAULT NULL,
PRIMARY KEY (id
),
UNIQUE KEY version_key
(app_id
, service_id
, version_id
)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
"""
CREATE_APPENGINE_INSTANCES_TABLE = """
CREATE TABLE {0}
(
id
bigint(20) unsigned NOT NULL AUTO_INCREMENT,
project_id
varchar(255) DEFAULT NULL,
app_id
varchar(255) DEFAULT NULL,
service_id
varchar(255) DEFAULT NULL,
version_id
varchar(255) DEFAULT NULL,
instance_id
varchar(255) DEFAULT NULL,
instance
json DEFAULT NULL,
PRIMARY KEY (id
),
UNIQUE KEY instance_key
(app_id
, service_id
, version_id
, instance_id
)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
"""
Then the pipeline can output the correct data to each table. That will be closer to the one row per resource that the new inventory will output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, will work on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. The only oddity is that it's easier to return the services, versions, instances data from retrieve() in loadable form. Versus forcing them into the app data, and then parse them out again in transform().
Codecov Report
@@ Coverage Diff @@
## dev #646 +/- ##
=========================================
+ Coverage 83.64% 83.7% +0.06%
=========================================
Files 165 165
Lines 8321 8354 +33
=========================================
+ Hits 6960 6993 +33
Misses 1361 1361
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This should be really useful.
self._get_loaded_count() | ||
|
||
# TODO: Make _get_loaded_count() support multiple resources in a single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You could put this under the if apps: since if there are no apps there won't be any other resources either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
After looking at the data in the responses, it seems the best way to incorporate the services/versions data is to associate the versions to each service, and then put the whole services payload into a json column named
services
. Of course theservices
payload is part of the raw json column.I will add testing if this approach is good. Otherwise, please let me know what else might work.