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

refactor/3052 refactor to avoid having too many files in root #3053

Conversation

davidberenstein1957
Copy link
Member

@davidberenstein1957 davidberenstein1957 commented Jun 1, 2023

Description

Moved some stuff

  • docker-related files to docker directory
  • scripts directory to docker directory
  • general files to .github

Closes #3052

Type of change

  • Refactor (change restructuring the codebase without changing functionality)

How Has This Been Tested

N.A.

Checklist

N.A.

@davidberenstein1957 davidberenstein1957 changed the base branch from main to develop June 1, 2023 09:08
@davidberenstein1957 davidberenstein1957 marked this pull request as ready for review June 1, 2023 09:09
@davidberenstein1957
Copy link
Member Author

@frascuchon we don't need to forget to set the read the docs config path to docs/_source/.readthedocs.yaml

@davidberenstein1957 davidberenstein1957 linked an issue Jun 1, 2023 that may be closed by this pull request
@davidberenstein1957
Copy link
Member Author

I tried to move the quickstart docker files to the .github folder too but it seemed that the build pipeline do not have access to .github.

.github/MANIFEST.in Outdated Show resolved Hide resolved
.github/.codecov.yml Outdated Show resolved Hide resolved
@frascuchon
Copy link
Member

As an easy step, what we can do is move all docker stuff (composes and dockerfiles) to a docker folder. Also, we can remove the commitlint configuration (since we're managing the changelog manually) and maybe the simpler ones to the .github folder too (README.md, CODE_OF_CONDUCT.md, LICENCE...)

@davidberenstein1957 davidberenstein1957 changed the title Feature/3052 refactor to avoid having too many files in root refactor/3052 refactor to avoid having too many files in root Jun 5, 2023
@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.37 ⚠️

Comparison is base (51751ac) 90.91% compared to head (1f5e67b) 90.54%.

❗ Current head 1f5e67b differs from pull request most recent head 4a894cd. Consider uploading reports for the commit 4a894cd to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3053      +/-   ##
===========================================
- Coverage    90.91%   90.54%   -0.37%     
===========================================
  Files          215      208       -7     
  Lines        11304    11120     -184     
===========================================
- Hits         10277    10069     -208     
- Misses        1027     1051      +24     
Flag Coverage Δ
pytest 90.54% <ø> (-0.37%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 26 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@davidberenstein1957
Copy link
Member Author

@frascuchon can you take a look at this PR?

@davidberenstein1957 davidberenstein1957 force-pushed the feature/3052-refactor-to-avoid-having-too-many-files-in-root branch from 0b28195 to f62823c Compare June 5, 2023 17:10
@davidberenstein1957 davidberenstein1957 changed the base branch from develop to main June 6, 2023 18:45
@davidberenstein1957
Copy link
Member Author

@frascuchon I changed the branch to main to avoid merge conflict with develop during the coming merges, and to avoid redirecting the .yml files to a non existend /main/docker/*.yml

Copy link
Member

@frascuchon frascuchon left a comment

Choose a reason for hiding this comment

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

Regarding this doc section, I'm not sure if we should move all those files into the dot-github folder.

It looks like this folder is used for templates and workflow definitions. It's a bit weird for me to find README.md or CHANGELOG.md there, instead of the root one. Even more for the conda environment environment_dev.yml or the coverage setup (.codecov.yml)

I would say:

  1. keep common repo files in the root folder (README.md CHANGELOG, coverage setup, conda env)
  2. Move al docker stuff into a docker folder and review the required scripts
  3. Remove unused files as the commitlint.config.js file or some obsolete script (I can take a look here)

Also, I think we should point this PR to the develop branch, right?

@frascuchon
Copy link
Member

I'm not sure if this checklist can be affected with these changes https://github.com/argilla-io/argilla/community

chore: added CONTRIBUTING.md
@davidberenstein1957 davidberenstein1957 changed the base branch from main to develop June 7, 2023 09:03
- added a call to action about contacting me or starting a PR
- added some default tags to bug and features
- added a separate issue for docs
- added UI/UX specific bug template
- added a python/deployment bug template
- added an integration issue template
@davidberenstein1957 davidberenstein1957 force-pushed the feature/3052-refactor-to-avoid-having-too-many-files-in-root branch from 77d6c6a to 47b94ba Compare June 7, 2023 15:21
davidberenstein1957 and others added 3 commits June 8, 2023 16:21
Co-authored-by: Francisco Aranda <francis@argilla.io>
# Description

The docker and script have been reviewed:

- The docker build context will be placed in the docker folder, so we
don't need to change anything inside the dockerfiles
- The unneeded scripts have been removed
- The specific docker script has been moved into docker/scripts
- The scripts used outside docker have been placed in the scripts
folder, otherwise, script execution may change. And not much sense to
put them inside the docker folder since they are used outside docker
images.
- Docs have been updated
- The `release.Dockerfile` has been renamed to `Dockerfile` (more
standard way for the principal docker image)

---------

Co-authored-by: davidberenstein1957 <david.m.berenstein@gmail.com>
docker/Dockerfile Outdated Show resolved Hide resolved
@frascuchon frascuchon self-requested a review June 21, 2023 15:52
Copy link
Member

@frascuchon frascuchon left a comment

Choose a reason for hiding this comment

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

It's working. Let's wait until the release branch creation before merging it, just to not disturb the release workflow

@davidberenstein1957 davidberenstein1957 force-pushed the feature/3052-refactor-to-avoid-having-too-many-files-in-root branch 3 times, most recently from 7761098 to 49e92ae Compare June 26, 2023 14:00
@davidberenstein1957
Copy link
Member Author

@frascuchon can you show me tomorrow how to resolve these conflicts?

@davidberenstein1957 davidberenstein1957 merged commit b6a6abe into develop Jun 27, 2023
@davidberenstein1957 davidberenstein1957 deleted the feature/3052-refactor-to-avoid-having-too-many-files-in-root branch June 27, 2023 14:01
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.

refactor . to avoid having too many files in root
2 participants