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

VCS: remove unused methods and make new Git pattern the default #8968

Merged
merged 19 commits into from
Aug 21, 2023

Conversation

humitos
Copy link
Member

@humitos humitos commented Feb 24, 2022

This PR removes the feature flag to use the new Git pattern, making it the default. Besides, it removes some methods that are not used anymore because we are always cloning the repository, so there is no need to update it or similar.

Closes #8940

Related

@stale

This comment was marked as outdated.

@stale stale bot added the Status: stale Issue will be considered inactive soon label May 2, 2022
@humitos humitos added Accepted Accepted issue on our roadmap and removed Status: stale Issue will be considered inactive soon labels May 2, 2022
@humitos
Copy link
Member Author

humitos commented May 2, 2022

There were some changes in our code, but this PR is still useful. I'll try to come back to it when I have some extra time.

@humitos humitos self-assigned this Aug 23, 2022
@humitos
Copy link
Member Author

humitos commented Feb 1, 2023

I updated this PR but there are still some extra work to do, so I'm leaving it again 😄

IMO, it would be good to deprecate and remove some VCS as already proposed (mainly SVN and bzr) to reduce the complexity and scope of this refactor.

Also, we should probably avoid keep using gitpython and go back to parse git's output (I think @stsewd already mentioned this in another issue). However, this should be simpler now because we are already doing this with ls-remote and it's working fine. Branches and tags will use the same command to get these. The only new command we will be introducing to parse its output would be git submodule to discover them.

@humitos humitos changed the title VCS: remove unused methods to "update" an existing copy VCS: remove unused methods and make new Git pattern the default Jul 31, 2023
Remove `GIT_CLONE_FETCH_CHECKOUT_PATTERN` feature flag and always use it. We
have been testing this new pattern and it has been working great.
@humitos
Copy link
Member Author

humitos commented Jul 31, 2023

I updated this PR again. The main new work here is that it makes the new Git pattern the default. I has been enabled for more than a week already and we haven't had any support issue about this. So, I think we are ready to remove the feature flag from our code.

IMO, it would be good to deprecate and remove some VCS as already proposed (mainly SVN and bzr) to reduce the complexity and scope of this refactor.

I'm still 👍🏼 on this, but I'm not going to cover it on this PR since it requires to follow our deprecation plan. I will do this work on #8840 in the next sprint.

Also, we should probably avoid keep using gitpython and go back to parse git's output

We are tracking this in another issue, so we will be following it there: https://github.com/readthedocs/readthedocs-corporate/issues/1524

@humitos humitos marked this pull request as ready for review July 31, 2023 11:15
@humitos humitos requested a review from a team as a code owner July 31, 2023 11:15
@humitos humitos requested a review from stsewd July 31, 2023 11:15
@humitos
Copy link
Member Author

humitos commented Aug 3, 2023

I will update this PR once Santos merges the work from #10594 so we don't compete for this chunk of code.

@humitos
Copy link
Member Author

humitos commented Aug 9, 2023

Blocked by #10606

@humitos humitos added the Status: blocked Issue is blocked on another issue label Aug 9, 2023
humitos added 4 commits August 9, 2023 16:03
We are not depending on GitPython anymore.
With the introduction of `git ls-remote` we don't require to parse the
repository anymore and GitPython is not required.

This commit also removes the methods `.branches` and `.tags` that are not used
anymore when using Git as VCS backend.
@humitos humitos removed the Status: blocked Issue is blocked on another issue label Aug 9, 2023
@humitos
Copy link
Member Author

humitos commented Aug 9, 2023

This is ready for review now 👍🏼

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

We should merge this after we have tested the new implementation more broadly again.

readthedocs/vcs_support/backends/git.py Outdated Show resolved Hide resolved
@emmettbutler
Copy link

Is it possible to tell a build to use the default prior to this change? It seems possible that this change broke dd-trace-py's release notes workflow. DataDog/dd-trace-py#7176

@stsewd
Copy link
Member

stsewd commented Oct 5, 2023

@emmettbutler it's not possible, if you need to unshallow you can follow the example from https://docs.readthedocs.io/en/stable/build-customization.html#unshallow-git-clone, or maybe the suggestion from #10795.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Build: cleanup VCS support to remove unused methods
3 participants