Skip to content
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

handle deployment_type updation of team_repos in update team repos api #449

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

adnanhashmi09
Copy link
Contributor

Purpose

This PR handles the updation of deployment_type field of TeamRepos. Frontend now sends deployment_type field in the request body.

Proposed changes (including videos or screenshots)

The RawOrgRepo dataclass has been extended to include deployment_type and the repo functions and service layer functions have been updated accordingly.

Comment on lines 124 to 128
idempotency_key_raw_org_repo_map = {
str(repo.idempotency_key): repo for repo in repos
}

team_repos = get_repository_service().update_team_repos(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Type hints

Comment on lines 124 to 139
idempotency_key_raw_org_repo_map = {
str(repo.idempotency_key): repo for repo in repos
}

team_repos = get_repository_service().update_team_repos(
team, repos, idempotency_key_raw_org_repo_map
)

updated_repo_response: List[Dict[str, Any]] = []
for repo in team_repos:
adapted_org_repo = adapt_org_repo(repo)
raw_repo = idempotency_key_raw_org_repo_map.get(str(repo.idempotency_key))
if raw_repo:
adapted_org_repo["deployment_type"] = raw_repo.deployment_type.value

updated_repo_response.append(adapted_org_repo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adnanhashmi09 so you are returning the same data for deployment type that you are getting from the client ? What if the data changes during the API call ?

Comment on lines 124 to 128
idempotency_key_raw_org_repo_map = {
str(repo.idempotency_key): repo for repo in repos
}

team_repos = get_repository_service().update_team_repos(
team, repos, idempotency_key_raw_org_repo_map
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if idempotency_key_raw_org_repo_map is generated from repos data, why are we even passing it down multiple levels of fucns down to the repo layer ? duplicate data being passed down funcs in diff format is not the best solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

Comment on lines +92 to +97
deployment_type=(
TeamReposDeploymentType(repo.get("deployment_type"))
if repo.get("deployment_type")
else TeamReposDeploymentType.PR_MERGE
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is RawOrgRepo data model? why does it have a TeamRepos property, deployment type?
If it is a data model for TeamRepos then why doesn't it have team_id and the name also tell that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it only had org repo data and no team repos data. But it was only being used with team repos based api and services. It would be better to restructure this model. I will do that in the next commit

Comment on lines 132 to 139
updated_repo_response: List[Dict[str, Any]] = []
for repo in team_repos:
adapted_org_repo = adapt_org_repo(repo)
raw_repo = idempotency_key_raw_org_repo_map.get(str(repo.idempotency_key))
if raw_repo:
adapted_org_repo["deployment_type"] = raw_repo.deployment_type.value

updated_repo_response.append(adapted_org_repo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to find a better way for this given that the codebase can have a data model for TeamRepos with relevant fields

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it would be better to get TeamRepos and then map

Comment on lines 46 to 51
def update_team_repos(
self,
team: Team,
org_repos: List[OrgRepo],
idempotency_key_raw_org_repo_map: Dict[str, RawOrgRepo] = {},
):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either pass data model for TeamRepos here or move this logic outside the repo layer. This is doing so much work that it is not responsible for

@adnanhashmi09 adnanhashmi09 dismissed amoghjalan’s stale review June 26, 2024 14:50

This has been updated

@adnanhashmi09 adnanhashmi09 merged commit 0328809 into main Jun 26, 2024
3 checks passed
@adnanhashmi09 adnanhashmi09 deleted the update-team-repos branch June 26, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants