-
Notifications
You must be signed in to change notification settings - Fork 10
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
add root_organization property to worker, clean up decoration service, change notification urls for GL #990
base: main
Are you sure you want to change the base?
Conversation
This PR includes changes to |
.filter_by(service_id=service_id, service=self.service) | ||
.one_or_none() | ||
) | ||
|
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.
this is not new - it's in shared, I'm just replicating it in worker.
I learned that in order to use shared's PlanService
, which now calls .root_organization
, sqlalchemy also needs to know about this property, so I had to add it.
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
root = None | ||
if self.service == "gitlab" and self.parent_service_id: | ||
root = self | ||
while root.parent_service_id is not None: |
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 think (but could be wrong) if self._get_owner_by_service
can return None
(from the .one_or_none()
it seems possible, there are no typehints in the functions) then you will have a NoneType has no property "parent_service_id"
error.
It would be good to add explicit typehints to the functions too
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.
Also, could we ever infinite loop because the parent_service_id is always None? Would we like to cap it after 10 iterations or something?
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.
added typehints, and changed that query to .one()
@giovanni-guidini
@adrian-codecov if parent_service_id
is None
the while
breaks - I think you mean the opposite, it will infinite loop if it's never None
. This condition would only happen if our data was bad and there was a set of Owners
referencing back and forth at each other. We don't have anything to prevent this, but it really should never happen. We have GL webhooks that should keep this field updated with the state on GL. I think the risk is low :)
@@ -31,8 +29,8 @@ class DecorationDetails(object): | |||
decoration_type: Decoration | |||
reason: str | |||
should_attempt_author_auto_activation: bool = False | |||
activation_org_ownerid: int = None | |||
activation_author_ownerid: int = None | |||
activation_org_ownerid: int | None = None |
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.
👍 ty
func.public.get_gitlab_root_group(org.ownerid) | ||
).first() | ||
# do not access plan directly - only through PlanService | ||
org_plan = PlanService(current_org=org) |
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.
The PlanService
doesn't seem to go looking for the root org (looking at main)
I think I'm missing a code change... does this PR depend on an unmerged change to shared too?
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.
Ah yes, codecov/shared#461 ok ok
@@ -50,7 +48,7 @@ def determine_uploads_used(plan_service: PlanService) -> int: | |||
|
|||
def determine_decoration_details( | |||
enriched_pull: EnrichedPull, empty_upload=None | |||
) -> dict: | |||
) -> DecorationDetails: |
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.
🏆
@@ -57,6 +58,16 @@ def get_dashboard_base_url() -> str: | |||
return configured_dashboard_url or "https://app.codecov.io" | |||
|
|||
|
|||
def _get_username_for_url(repository: Repository) -> str: | |||
username = repository.owner.username | |||
if repository.owner.service == Service.GITLAB.value: |
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.
Idk if we save service always as "gitlab" vs "GitLab" or something like that, might be worth lowercasing this just in case
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.
it follows the enum values in Service
, just checked metabase, it will only be gitlab
df7068f
to
bb01103
Compare
This PR includes changes to |
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Lgtm, waiting on the Shared update and can approve then 👌 |
bb01103
to
b1ea58a
Compare
b1ea58a
to
a6005df
Compare
Decoration service was causing confusion for GitLab users. Cleaned it up and added some comments to clarify the current GL org strategy which is:
plan_activated_user
also happens on the root org instead of a subgroup orgplan
ormember
url, give GL users the correct url (correct url is to root group, not subgroup)more details on ticket codecov/engineering-team#2710