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

Remove the Git repository synchronization functionality #6904

Merged
merged 6 commits into from
Sep 29, 2023

Conversation

SpecLad
Copy link
Contributor

@SpecLad SpecLad commented Sep 26, 2023

Motivation and context

This functionality has accumulated significant technical debt:

  • Most importantly, it does not use the current authorization system,
    rendering it accessible only for admin users.

  • It doesn't follow the regular API conventions, and is not visible in the API
    schema. This necessitates special code in the SDK.

  • The initialization code in base.py is not safe when multiple instances of
    the server start at the same time (each instance may end up generating its
    own key).

The team has decided that the cost of fixing these issues outweighs the benefit
of the functionality, so remove it.

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • I have added a description of my changes into the CHANGELOG file
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • [ ] I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

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.

@SpecLad SpecLad force-pushed the remove-git-sync branch 2 times, most recently from 5acf843 to 9c9726f Compare September 26, 2023 14:30
@SpecLad
Copy link
Contributor Author

SpecLad commented Sep 26, 2023

/check

@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2023

❌ Some checks failed
📄 See logs here

This functionality has accumulated significant technical debt:

* Most importantly, it does not use the current authorization system,
  rendering it accessible only for admin users.

* It doesn't follow the regular API conventions, and is not visible in the API
  schema. This necessitates special code in the SDK.

* The initialization code in `base.py` is not safe when multiple instances of
  the server start at the same time (each instance may end up generating its
  own key).

The team has decided that the cost of fixing these issues outweighs the benefit
of the functionality, so remove it.
@SpecLad
Copy link
Contributor Author

SpecLad commented Sep 26, 2023

/check

@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2023

✔️ All checks completed successfully
📄 See logs here

@SpecLad SpecLad marked this pull request as ready for review September 27, 2023 09:55
@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #6904 (b2a7112) into develop (4a487c3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           develop    #6904    +/-   ##
=========================================
  Coverage    82.52%   82.52%            
=========================================
  Files          370      360    -10     
  Lines        39869    38908   -961     
  Branches      3560     3544    -16     
=========================================
- Hits         32902    32110   -792     
+ Misses        6967     6798   -169     
Components Coverage Δ
cvat-ui 77.62% <100.00%> (+0.06%) ⬆️
cvat-server 87.00% <ø> (+0.11%) ⬆️

Copy link
Contributor

@nmanovic nmanovic left a comment

Choose a reason for hiding this comment

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

LGTM

@nmanovic nmanovic merged commit d497bb6 into cvat-ai:develop Sep 29, 2023
32 checks passed
@SpecLad SpecLad deleted the remove-git-sync branch October 9, 2023 15:51
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Oct 25, 2023
This functionality has accumulated significant technical debt:

* Most importantly, it does not use the current authorization system,
  rendering it accessible only for admin users.

* It doesn't follow the regular API conventions and is not visible in
the API
  schema. This necessitates a special code in the SDK.

* The initialization code in `base.py` is not safe when multiple
instances of
the server starts at the same time (each instance may end up generating
its
  own key).

The team has decided that the cost of fixing these issues outweighs the benefit of the functionality, so remove it.
@PMazarovich
Copy link
Contributor

Hello!
Are there any plans to restore it in the future?
Thanks!

@nmanovic
Copy link
Contributor

@PMazarovich , you are the only known user of the feature. Could you please describe how have you used the feature?

@PMazarovich
Copy link
Contributor

PMazarovich commented Jan 26, 2024

@nmanovic here it is:

  1. manually push the "synchronise" button on CVAT
  2. git pipeline was launched
  3. creating a new branch with the new data
  4. running some script to count number of errors
  5. data moved to "valid" or "unvalid" folder depending on threshold value
  6. push the data from "valid folder" to AWS S3

@dpovision
Copy link

IMHO it was a nice automatic backup feature and I would prefer it to other methods like manually created backups by admins or even users. I understand your decision "cost of fixing vs. benefit of functionality" but nevertheless it was a useful feature and I would vote for it. :-)

@nmanovic
Copy link
Contributor

nmanovic commented Feb 1, 2024

@nmanovic here it is:

  1. manually push the "synchronise" button on CVAT
  2. git pipeline was launched
  3. creating a new branch with the new data
  4. running some script to count number of errors
  5. data moved to "valid" or "unvalid" folder depending on threshold value
  6. push the data from "valid folder" to AWS S3

@PMazarovich

Can it be implemented using web hooks? https://opencv.github.io/cvat/docs/administration/advanced/webhooks/

nmanovic pushed a commit that referenced this pull request Mar 22, 2024
This was only used by the Git synchronization functionality, which was
removed in #6904.
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.

5 participants