feat: add gooddata-pipelines package #1074
Conversation
janmatzek
force-pushed
the
jmat-services-pipelines-packages
branch
from
e50c7cf to
d79baa1
Compare
janmatzek
force-pushed
the
jmat-services-pipelines-packages
branch
2 times, most recently
from
3d4e46e to
b00dc0f
Compare
janmatzek
requested review from
hkad98,
jaceksan,
lupko and
pcerny
as code owners
hkad98
left a comment
hkad98
left a comment
There was a problem hiding this comment.
I added some comments; please have a look. Take them as optional.
I also noticed and cannot add a comment – .gitignore. Why do we need this .gitignore? Other directories have .gitkeep.
...ta-pipelines/gooddata_pipelines/provisioning/entities/user_data_filters/models/udf_models.py
Show resolved
Hide resolved
gooddata-pipelines/gooddata_pipelines/provisioning/entities/users/models/permissions.py
Show resolved
Hide resolved
| id = ( | ||
| permission["user_id"] | ||
| if permission["user_id"] | ||
| else permission["ug_id"] | ||
| ) | ||
|
|
||
| if permission["user_id"]: | ||
| target_type = PermissionType.user | ||
| else: | ||
| target_type = PermissionType.user_group | ||
|
|
||
| permissions.append( | ||
| PermissionIncrementalLoad( | ||
| permission=permission["ws_permissions"], | ||
| workspace_id=permission["ws_id"], | ||
| id=id, | ||
| type=target_type, | ||
| is_active=str(permission["is_active"]).lower() == "true", | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Proposed simplification:
user_id = permission.get("user_id")
user_group_id = permission.get("user_id")
assert user_id is not None and user_group_id is not None
if user_id is not None:
target_type = PermissionType.user
else:
target_type = PermissionType.user_group
id = user_id or user_group_id
Consider using get for dictionaries. You can specify default value as an argument. E.g. permission.get("something", "nothing found")
There was a problem hiding this comment.
Good point, these could definitely be more readable! I updated one of the models and made a TODO. Refactoring this logic is in the task list anyway so I would do the cleanup toghether with that.
gooddata-pipelines/gooddata_pipelines/provisioning/entities/users/models/users.py
Show resolved
Hide resolved
| export_path = ( | ||
| self._backup_path | ||
| + org_id | ||
| + "/" | ||
| + full_path[len(folder) + 1 :] | ||
| + "/" | ||
| ) |
|
|
||
| def export(self, folder, org_id) -> None: | ||
| """Uploads the content of the folder to S3 as backup.""" | ||
| storage_path = self._config.bucket + "/" + self._backup_path |
| export_path = ( | ||
| self._backup_path | ||
| + org_id | ||
| + "/" | ||
| + full_path[len(folder) + 1 :] | ||
| ) |
There was a problem hiding this comment.
Consider using f-string + is it necessary to init this within the for cycle? I cannot see how export_path changes.
gooddata-pipelines/pyproject.toml
Outdated
| version = "0.1.0" | ||
| description = "" | ||
| authors = [{ name = "GoodData", email = "support@gooddata.com" }] | ||
| license = { text = "BSD" } |
There was a problem hiding this comment.
We are still not sure about this one.
There was a problem hiding this comment.
Just got confirmation from Lenka that they (as in SPACE) are ok with keeping this one as the the goodata-productivity-tools was approved with this previously
| if isinstance(value, list): | ||
| attrs[key] = ", ".join( | ||
| str(list_item) for list_item in value | ||
| ) | ||
| elif value is None: | ||
| continue | ||
| else: | ||
| attrs[key] = str(value) |
There was a problem hiding this comment.
Proposed for better readability:
if value is None:
continue
if isinstance(value, list):
attrs[key] = ", ".join(str(list_item) for list_item in value)
else:
attrs[key] = str(value)
janmatzek
force-pushed
the
jmat-services-pipelines-packages
branch
from
2259f90 to
7215918
Compare
|
I just realized one more thing. We use tbump to bump the version for packages. Please include the new package there as well. |
janmatzek
force-pushed
the
jmat-services-pipelines-packages
branch
from
4a412f6 to
d93e26d
Compare
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1074 +/- ##
==========================================
- Coverage 83.07% 82.96% -0.12%
==========================================
Files 158 158
Lines 10762 10828 +66
==========================================
+ Hits 8941 8983 +42
- Misses 1821 1845 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
janmatzek
force-pushed
the
jmat-services-pipelines-packages
branch
from
7461b8a to
de0403d
Compare
janmatzek
force-pushed
the
jmat-services-pipelines-packages
branch
from
de0403d to
3a45cf7
Compare
3 participants
No description provided.