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

[cvat-core] support cloud storage #3313

Merged
merged 21 commits into from
Sep 2, 2021
Merged

Conversation

Marishka17
Copy link
Contributor

@Marishka17 Marishka17 commented Jun 10, 2021

Motivation and context

Related issue: #863
TODOs:

How has this been tested?

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 requested a review from bsekachev June 10, 2021 12:00
@bsekachev bsekachev self-assigned this Jun 11, 2021
cvat-core/src/api.js Outdated Show resolved Hide resolved
cvat-core/src/api.js Outdated Show resolved Hide resolved
cvat-core/src/api.js Outdated Show resolved Hide resolved
cvat-core/src/api.js Outdated Show resolved Hide resolved
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/cloud-storage.js Outdated Show resolved Hide resolved
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/cloud-storage.js Outdated Show resolved Hide resolved
@bsekachev
Copy link
Member

The cvat-core part looks fine. Let's wait for server part to be merged and during UI development see if the API should be adjusted for convenience.

@Marishka17 Marishka17 mentioned this pull request Jun 28, 2021
22 tasks
@Marishka17 Marishka17 changed the title UI support cloud storage [UI] [cvat-core] support cloud storage Jun 28, 2021
@Marishka17 Marishka17 marked this pull request as ready for review August 10, 2021 13:43
@Marishka17 Marishka17 requested a review from bsekachev August 11, 2021 09:50
@bsekachev
Copy link
Member

There are Tests in TODO list. Are you planning to implement them?

bsekachev
bsekachev previously approved these changes Aug 12, 2021
Copy link
Member

@bsekachev bsekachev left a comment

Choose a reason for hiding this comment

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

I do not have important ideas about this PR. Actually I already reviewed it at the most. Let's wait till server is merged and then I am ready to merge it.

Comment on lines 1571 to 1576
set: (cloudStorageId) => {
if (!Number.isInteger(cloudStorageId) || cloudStorageId <= 0) {
throw new ArgumentError('Value must be a positive integer');
}
data.cloud_storage_id = cloudStorageId;
},
Copy link
Member

Choose a reason for hiding this comment

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

Since we can't change cloudStorageId, I would suggest to do this field readonly (cloud_storage_id is set via constructor now). What do you think?

@bsekachev bsekachev changed the title [UI] [cvat-core] support cloud storage [Dependend] [cvat-core] support cloud storage Aug 12, 2021
@bsekachev bsekachev changed the title [Dependend] [cvat-core] support cloud storage [cvat-core] support cloud storage Aug 31, 2021
Comment on lines 1547 to 1552
// set: (cloudStorageId) => {
// if (!Number.isInteger(cloudStorageId) || cloudStorageId <= 0) {
// throw new ArgumentError('Value must be a positive integer');
// }
// data.cloud_storage_id = cloudStorageId;
// },
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this commented code?

@Marishka17 Marishka17 changed the title [cvat-core] support cloud storage [WIP][cvat-core] support cloud storage Sep 1, 2021
@Marishka17
Copy link
Contributor Author

There are Tests in TODO list. Are you planning to implement them?

yes, I will add tests

@Marishka17 Marishka17 requested a review from nmanovic as a code owner September 2, 2021 08:09
@Marishka17 Marishka17 changed the title [WIP][cvat-core] support cloud storage [cvat-core] support cloud storage Sep 2, 2021
@Marishka17 Marishka17 requested a review from bsekachev September 2, 2021 09:10
@bsekachev bsekachev merged commit 720d798 into develop Sep 2, 2021
@bsekachev bsekachev deleted the mk/ui_support_cloud_storage branch September 2, 2021 11:47
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.

2 participants