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

Deprecate blobs_get and open blobs_get_v2 #183

Merged
merged 6 commits into from
Nov 28, 2023

Conversation

ddl-giuliocapolino
Copy link
Contributor

@ddl-giuliocapolino ddl-giuliocapolino commented Oct 27, 2023

Link to JIRA

DOM-51572

What issue does this pull request solve?

blobs_get needs to be deprecated in favor of a more secure endpoint. This will not need to go in 5.9 - so the PR will stay open for some time.

What is the solution?

deprecate it and add new endpoint!

Testing

Briefly describe how the change was tested. The purpose of this section is that a reviewer can identify a test gap, if any.

e.g. "I ran an upgrade from 4.2 to 4.6".

  • Unit test(s)

Pull Request Reminders

References (optional)

@ddl-giuliocapolino ddl-giuliocapolino force-pushed the ddl-giuliocapolino.blob-get-deprecation branch from 11840b4 to 9a3998b Compare November 2, 2023 23:45
@ddl-giuliocapolino ddl-giuliocapolino force-pushed the ddl-giuliocapolino.blob-get-deprecation branch from 9a3998b to a8ed6fc Compare November 2, 2023 23:56
@ddl-giuliocapolino ddl-giuliocapolino marked this pull request as ready for review November 3, 2023 00:45
@ddl-giuliocapolino ddl-giuliocapolino requested review from a team November 3, 2023 00:45
Copy link

@ddl-aj-rossman ddl-aj-rossman left a comment

Choose a reason for hiding this comment

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

LGTM

domino/routes.py Outdated
def blobs_get(self, key):
return self._build_project_url() + "/blobs/" + key

def blobs_get_v2(self, path, commit_id, project_id):
return self.host + f"/api/projects/beta/projects/{project_id}/files/{commit_id}/{path}/content"

Choose a reason for hiding this comment

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

should this be /v1/ instead of /beta/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh good catch! yes once I merge in the other PR I will change this to it

@ddl-giuliocapolino ddl-giuliocapolino force-pushed the ddl-giuliocapolino.blob-get-deprecation branch from 0956e72 to 6a8a10b Compare November 14, 2023 20:50
@@ -69,7 +69,7 @@ def download_results(domino, commitId, results, output, exp):
output = ""

for d in download:
url = domino.blobs_get(d["key"])
url = domino.blobs_get_v2(d["path"], commitId, domino.project_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tested this and works as expected!

@@ -1203,6 +1217,17 @@ def _validate_blob_key(key):
)
)

@staticmethod
def _validate_blob_path(path):
Copy link
Contributor

Choose a reason for hiding this comment

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

would love some test and a docstring explaining what this does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added! :)

@ddl-olsonJD
Copy link
Contributor

Looks good. Update the changelog with the changes.

@ddl-olsonJD ddl-olsonJD self-requested a review November 28, 2023 21:29
@ddl-olsonJD ddl-olsonJD merged commit d361b6f into master Nov 28, 2023
This pull request was closed.
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.

4 participants