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

[ENG-6035] | Bitbucket Storage Addon Implementation #135

Merged

Conversation

sh-andriy
Copy link
Collaborator

@sh-andriy sh-andriy commented Oct 24, 2024

Bitbucket Storage Addon Implementation PR | [ENG-6035]

@sh-andriy sh-andriy requested a review from opaduchak October 24, 2024 14:18
Comment on lines 43 to 56
async with self.network.GET("user/permissions/workspaces") as response:
data = await response.json_content()
workspaces = data.get("values", [])
if not workspaces:
raise ValueError("No workspaces found")

for workspace in workspaces:
slug = workspace["workspace"]["slug"]
perm = workspace.get("permission")
if perm in ["admin", "contributor", "member", "owner"]:
self.workspace_slug = slug
break
else:
raise ValueError("No suitable workspace found")
Copy link
Collaborator

Choose a reason for hiding this comment

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

may you explain the reason behind this?
Because uuid is being obtained higher and isn't manipulated on commented lines

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, so basically the reason for fetching the user's workspaces and setting self.workspace_slug is that we need to identify a workspace where the user has sufficient permissions (admin, contributor, member, or owner) to perform subsequent API operations. The uuid obtained earlier identifies the user but doesn't provide the necessary context for workspace-specific actions. By selecting an appropriate workspace_slug, we ensure that our API requests target a workspace the user can access.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I get where you are coming from but I still have my reservations:

  1. Won't workspace slug be lost if you set it as a property of addon imp?
  2. If you have multiple workpaces you overwrite workspace slug with the last one in response.
  3. I'd suggest to work with workspaces as with top-level folders (aka root items) and include them in navigation

Copy link
Collaborator Author

@sh-andriy sh-andriy Oct 31, 2024

Choose a reason for hiding this comment

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

You're right, appreciate you pointing this out. Working on this changes

Comment on lines 99 to 100
except Exception as e:
raise ValueError(f"Failed to fetch item info: {e}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

http client will raise only when it obtains 50* status code, I'd advice to handle 40* status codes instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. I'll update the code to explicitly handle 4xx status codes, since the HTTP client only raises exceptions for 5xx errors.

Copy link
Collaborator

@opaduchak opaduchak left a comment

Choose a reason for hiding this comment

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

Good Job!
Nothing much to say, mostly cosmetic requests

Comment on lines 153 to 124
repo_full_name, path_param = self._split_repo_full_name_and_path(actual_id)
endpoint = f"repositories/{repo_full_name}/src/HEAD/{path_param}"
try:
async with self.network.GET(endpoint, query=params) as response:
json_data = await self._handle_response(response)
except Exception as e:
raise ValueError(f"Failed to list child items: {e}")
items = []
for item in json_data.get("values", []):
item_type_value = (
storage.ItemType.FOLDER
if item["type"] == "commit_directory"
else storage.ItemType.FILE
)
if item_type is not None and item_type != item_type_value:
continue
path = item.get("path")
if not path:
continue
item_name = path.split("/")[-1] or "Unnamed Item"
item_id_value = self._make_item_id(
"repository", f"{repo_full_name}/{path}"
)
items.append(
storage.ItemResult(
item_id=item_id_value,
item_name=item_name,
item_type=item_type_value,
)
)
result = storage.ItemSampleResult(items=items)
next_cursor = json_data.get("next")
cursor = NextLinkCursor(next_link=next_cursor)
result = result.with_cursor(cursor)
return result
else:
raise ValueError(f"Cannot list child items for item type {item_type_str}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, extract this to separate method as list_child_items becomes too long

Comment on lines 124 to 151
workspace_slug = actual_id
params["role"] = "member"
params["pagelen"] = "100"
endpoint = f"repositories/{workspace_slug}"
try:
async with self.network.GET(endpoint, query=params) as response:
json_data = await self._handle_response(response)
except Exception as e:
raise ValueError(f"Failed to fetch repositories: {e}")
items = []
for repo in json_data.get("values", []):
full_name = repo.get("full_name")
name = repo.get("name") or "Unnamed Repository"
if not full_name:
continue
item_id = self._make_item_id("repository", full_name)
items.append(
storage.ItemResult(
item_id=item_id,
item_name=name,
item_type=storage.ItemType.FOLDER,
)
)
result = storage.ItemSampleResult(items=items)
next_cursor = json_data.get("next")
cursor = NextLinkCursor(next_link=next_cursor)
result = result.with_cursor(cursor)
return result
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also extract this, please

Comment on lines 183 to 187
result = storage.ItemSampleResult(items=items)
next_cursor = json_data.get("next")
cursor = NextLinkCursor(next_link=next_cursor)
result = result.with_cursor(cursor)
return result
Copy link
Collaborator

Choose a reason for hiding this comment

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

this may be deduplicated

@sh-andriy sh-andriy requested a review from opaduchak November 7, 2024 13:08
@brianjgeiger brianjgeiger merged commit 128f3a9 into CenterForOpenScience:develop Nov 8, 2024
1 check passed
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.

3 participants