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

Support GCS #3919

Merged
merged 29 commits into from
Nov 26, 2021
Merged

Support GCS #3919

merged 29 commits into from
Nov 26, 2021

Conversation

Marishka17
Copy link
Contributor

@Marishka17 Marishka17 commented Nov 18, 2021

Motivation and context

Resolve #3760

How has this been tested?

Manually + extended jest tests

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) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

@Marishka17 Marishka17 marked this pull request as ready for review November 23, 2021 08:57
@Marishka17 Marishka17 linked an issue Nov 23, 2021 that may be closed by this pull request
2 tasks
provider_type = models.CharField(max_length=20, choices=CloudProviderChoice.choices())
resource = models.CharField(max_length=63)
resource = models.CharField(max_length=222)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmanovic, I've increased restrictions for bucket names length because google cloud storage has 2 restriction variants - 63 like other providers and 222 with dots in bucket name. Are we going to support the second case?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Marishka17 , you can even have 255 (just in case). I really don't think that we have to be very strict here.

cvat-core/src/cloud-storage.js Outdated Show resolved Hide resolved
cvat-core/src/cloud-storage.js Outdated Show resolved Hide resolved
cvat-core/src/server-proxy.js Outdated Show resolved Hide resolved
cvat-ui/src/assets/google-cloud.svg Outdated Show resolved Hide resolved
@@ -170,15 +198,37 @@ export default function CreateCloudStorageForm(props: Props): JSX.Element {
duration: 15,
});
}
}, [cloudStorage]);
}, [cloudStorageId]);
Copy link
Member

Choose a reason for hiding this comment

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

effect callback also depends on cloudStorage

className: 'cvat-incorrect-add-region-notification',
});
} else {
const regionsCopy = regions;
Copy link
Member

Choose a reason for hiding this comment

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

I hardly think it is a copy, that is probably link to the same object. Check if regions contains newRegionKey after it was inserted to regionsCopy.

Copy link
Member

Choose a reason for hiding this comment

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

If you need exactly copy, maybe better to use Object and create a shallow copy this way: const copy = { ...source }

Marishka17 and others added 2 commits November 24, 2021 16:01
…-form.tsx

Co-authored-by: Boris Sekachev <boris.sekachev@intel.com>
…-form.tsx

Co-authored-by: Boris Sekachev <boris.sekachev@intel.com>
@Marishka17 Marishka17 changed the title Support GCS [WIP] Support GCS Nov 24, 2021
@bsekachev
Copy link
Member

Looks like the patch works for me. But maybe one reccomendation.
After I select a key file, the field is empty anyway
image

Probably better to show there file name.

@Marishka17 Marishka17 changed the title [WIP] Support GCS Support GCS Nov 26, 2021
@Marishka17
Copy link
Contributor Author

@nmanovic, Could you please take a look at the server part of this PR?

@@ -809,6 +811,7 @@ class CloudStorageSerializer(serializers.ModelSerializer):
key = serializers.CharField(max_length=20, allow_blank=True, required=False)
secret_key = serializers.CharField(max_length=40, allow_blank=True, required=False)
key_file_path = serializers.CharField(max_length=64, allow_blank=True, required=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

@Marishka17 , I believe key_file_path has to be removed. What do you think? Probably you can do that in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I added the option to upload a key file for a case when CVAT instance is deployed remotely. But now I've looked at it from a different side. Ok, I'll prepare PR for this.

@nmanovic nmanovic merged commit f59d1f5 into develop Nov 26, 2021
@nmanovic nmanovic deleted the mk/support_gcs branch November 26, 2021 12:41
@Marishka17 Marishka17 mentioned this pull request Nov 29, 2021
5 tasks
sarikaya added a commit to roboduels/cvat that referenced this pull request Dec 4, 2021
…o openvinotoolkit-develop

* 'develop' of https://github.com/openvinotoolkit/cvat: (181 commits)
  Cypress. Update case 105. Add check "Google cloud storage" provider fields. (cvat-ai#3980)
  Cypress. Fix case 108 for Firefox. (cvat-ai#3981)
  update project section (cvat-ai#3979)
  Cypress. Test "Rotated bounding boxes" (cvat-ai#3961)
  Issue deleting (cvat-ai#3952)
  Remove key_file_path field (cvat-ai#3959)
  update the doc for change default hostname or port (cvat-ai#3915)
  CVAT_server. Test for "Project updated time". (cvat-ai#3953)
  fix: cvat/requirements/base.txt to reduce vulnerabilities (cvat-ai#3970)
  Fix notification (cvat-ai#3960)
  Update documentation for China users (cvat-ai#3946)
  Support GCS (cvat-ai#3919)
  Cypress. Update the test for check issue 3810. (cvat-ai#3900)
  Update the documentation about the smooth image option (cvat-ai#3947)
  Project tasks pagination (cvat-ai#3910)
  Update documentation about review mode (cvat-ai#3944)
  Preserve the order of the label attributes in the object item details. (cvat-ai#3945)
  Added smooth image option (cvat-ai#3933)
  Fixed issue: autoborder points are visible for invisible shapes (cvat-ai#3931)
  Add FiftyOne to partners list (cvat-ai#3943)
  ...
sarikaya added a commit to roboduels/cvat that referenced this pull request Dec 4, 2021
* openvinotoolkit-develop: (181 commits)
  Cypress. Update case 105. Add check "Google cloud storage" provider fields. (cvat-ai#3980)
  Cypress. Fix case 108 for Firefox. (cvat-ai#3981)
  update project section (cvat-ai#3979)
  Cypress. Test "Rotated bounding boxes" (cvat-ai#3961)
  Issue deleting (cvat-ai#3952)
  Remove key_file_path field (cvat-ai#3959)
  update the doc for change default hostname or port (cvat-ai#3915)
  CVAT_server. Test for "Project updated time". (cvat-ai#3953)
  fix: cvat/requirements/base.txt to reduce vulnerabilities (cvat-ai#3970)
  Fix notification (cvat-ai#3960)
  Update documentation for China users (cvat-ai#3946)
  Support GCS (cvat-ai#3919)
  Cypress. Update the test for check issue 3810. (cvat-ai#3900)
  Update the documentation about the smooth image option (cvat-ai#3947)
  Project tasks pagination (cvat-ai#3910)
  Update documentation about review mode (cvat-ai#3944)
  Preserve the order of the label attributes in the object item details. (cvat-ai#3945)
  Added smooth image option (cvat-ai#3933)
  Fixed issue: autoborder points are visible for invisible shapes (cvat-ai#3931)
  Add FiftyOne to partners list (cvat-ai#3943)
  ...
@nmanovic nmanovic mentioned this pull request Mar 4, 2022
7 tasks
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.

Support Google Cloud Storage in UI
3 participants