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.

Sign up for GitHub

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

Jump to bottom

Add support for Azure Blob Storage connection string authentication #4649

Merged
merged 21 commits into from
Mar 21, 2023
Merged

Add support for Azure Blob Storage connection string authentication #4649

merged 21 commits into from
Mar 21, 2023

Conversation

suzusuzu
Copy link
Contributor

@suzusuzu suzusuzu commented May 20, 2022 •
edited
Loading

Motivation and context

This is a change to support Connection String authentication for Azure Blob Storage. For example, Connection String Authentication allows for Azure Blob Storage running on the IoT Edge of an edge device.

How has this been tested?

I tested it by actually running it as shown in the next image, I could not find where the E2E test for cloudstorage was done.
cvat_blob_connection_string

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2022 Intel Corporation
#
# SPDX-License-Identifier: MIT

@suzusuzu suzusuzu changed the title Feature/blob connection string Add support for Azure Blob Storage connection string authentication May 20, 2022
@Marishka17
Copy link
Contributor

Hi, @suzusuzu, thanks for the contribution! Are you still going to contribute to us? If so, could you please resolve conflicts in this PR?

@suzusuzu
Copy link
Contributor Author

@Marishka17 Thanks for the reply, I resolved the conflict. Please check it out.

@Marishka17 Marishka17 self-requested a review August 22, 2022 20:47
@Marishka17 Marishka17 self-assigned this Aug 22, 2022
@bsekachev
Copy link
Member

/check

@github-actions
Copy link
Contributor

github-actions bot commented Sep 12, 2022 •
edited
Loading

❌ Some checks failed
📄 See logs here

@cvat-ai cvat-ai deleted a comment from github-actions bot Sep 12, 2022
@bsekachev
Copy link
Member

@kirill-sizov Can you look to full-check run? Looks like it does not work for 3rd-party PRs.

@suzusuzu suzusuzu force-pushed the feature/blob_connection_string branch from bd456b2 to b9dcd85 Compare October 13, 2022 06:47
@sizov-kirill
Copy link
Contributor

/check

@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2022 •
edited
Loading

✔️ All checks completed successfully
📄 See logs here

@sizov-kirill
Copy link
Contributor

@Marishka17 Have you already reviewed this PR? Or it requires additional review?

@Marishka17
Copy link
Contributor

Unfortunately, I haven't, but that was planned. Need to review and test it with a container on Azure.

@nmanovic
Copy link
Contributor

@Marishka17 , let's try to find sometime next week to review the PR.

@Marishka17
Copy link
Contributor

@nmanovic, Ok, but in this case, I need an account on Azure for testing. I forgot to ask this again in our last meeting about this question.

Marishka17
Marishka17 previously requested changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@Marishka17 Marishka17 left a comment

Choose a reason for hiding this comment

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

@suzusuzu, Thanks for the contribution!
Could you please fix several minor comments and also update cvat-core/src/cloud-storage.ts implementation by adding a connection string like other attributes implementation? (otherwise, the connection string will not be transferred from the client to the server)

cvat-ui/src/components/create-cloud-storage-page/cloud-storage-form.tsx Outdated Show resolved Hide resolved
cvat/apps/engine/cloud_provider.py Outdated Show resolved Hide resolved
cvat/apps/engine/models.py Show resolved Hide resolved
cvat/apps/engine/serializers.py Outdated Show resolved Hide resolved
site/content/en/docs/manual/basics/attach-cloud-storage.md Outdated Show resolved Hide resolved
@Marishka17
Copy link
Contributor

Marishka17 commented Jan 17, 2023 •
edited
Loading

@suzusuzu, One more thing, when I attach the Azure container and go to update cloud storage page field with connection string is empty.(Need add fake credentials for connection string.)

@nmanovic
Copy link
Contributor

@SpecLad , your check is working :). Should @Marishka17 just update it in the case?

@Marishka17 Marishka17 removed the request for review from SpecLad March 19, 2023 14:19
zhiltsov-max
zhiltsov-max reviewed Mar 20, 2023
View reviewed changes
cvat/schema.yml Outdated Show resolved Hide resolved
@nmanovic nmanovic dismissed Marishka17’s stale review March 21, 2023 10:24

All issues were fixed

@nmanovic nmanovic merged commit 4ae8bdc into cvat-ai:develop Mar 21, 2023
@SpecLad SpecLad mentioned this pull request Mar 27, 2023
Merged
2 tasks
nmanovic pushed a commit that referenced this pull request Mar 27, 2023
@SpecLad
Resolve migration conflict (#5939)
79c3fbc 
Merging both #4649 and #5855 broke migrations.
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Jul 1, 2023
@suzusuzu @mikhail-treskin
Add support for Azure Blob Storage connection string authentication (c…
6174c73 
…vat-ai#4649)

This is a change to support Connection String authentication for Azure
Blob Storage. For example, Connection String Authentication allows for
Azure Blob Storage running on the IoT Edge of an edge device.

### How has this been tested?
<!-- Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc. -->

I tested it by actually running it as shown in the next image, I could
not find where the E2E test for cloudstorage was done.
<img width="1001" alt="cvat_blob_connection_string"
src="https://user-images.githubusercontent.com/10334593/169458508-cfa4030a-578f-4aad-bfd9-fa01c9ca8230.png">
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Jul 1, 2023
@SpecLad @mikhail-treskin
Resolve migration conflict (cvat-ai#5939)
7af7dc6 
Merging both cvat-ai#4649 and cvat-ai#5855 broke migrations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhiltsov-max zhiltsov-max zhiltsov-max approved these changes

@Marishka17 Marishka17 Marishka17 left review comments

Assignees

@Marishka17 Marishka17

Labels
data@cloud
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@suzusuzu @Marishka17 @bsekachev @sizov-kirill @nmanovic @zhiltsov-max