Build: replace GitPython with git commands #10594

stsewd · 2023-08-02T01:23:14Z

To unambiguously parse the submodules, we need to use the --null option, that results on each item being separated by a null character, all good, except that we are "sanitizing" the output of all commands, stripping those null characters. We are doing this to avoid problems when saving this in the DB, so instead of "sanitizing" the output after the command is run, I'm moving that logic to just when it's needed, this is when saving the command using the API.

Closes https://github.com/readthedocs/readthedocs-corporate/issues/1524
Closes #8990

stsewd · 2023-08-02T21:16:38Z

readthedocs/vcs_support/backends/git.py

+@dataclass(slots=True)
+class GitSubmodule:
+    path: str
+    url: str


We need the path and URL of each submodule, since we validate that each URL has a valid/supported schema, we could remove this check if we want, it would simplify the code a little :)

And we would just need to call Git.submodules when the user is excluding submodules only, when including some or all, we can just call the git submodule update with an empty list or the list of given submodules.

And if the user provides submodules that aren't valid, well, git will just take care of showing the correct error message, right now we are just excluding invalid submodules from the list.

Do you have a preference here?

I don't remember why we were checking if the URL has a supported scheme. Maybe that will help me to have a more informed opinion 😄

@agjohnson may have some input here. From what I see, this was done, so users wouldn't use private submodules (SSH) .

Thinking about this more, I seem to recall some issues with repositories defining bogus submodules, or having old and unused submodules defined. I don't think this was to limit the URL pattern to supported protocols.

It could have also been a security measure, as it was probably possible to define a submodule like /etc/ or something and clone from something outside the container (when we were performing submodule operations outside containers).

Any of this seem familiar or likely?

Stealing internal repos could have been a possibility before moving the execution to docker.

It sounds familiar to me the validation of the submodules, but this PR should fix that automatically because we trust git itself now

Yeah, I'm thinking if any of the above are true, it was probably a throw back to when we weren't executing the submodule steps inside Docker. In any case that would seem this logic can be removed, but if anyone is hesitant about it, I'm not saying this is guaranteed to be safe either.

In that case, I'd say this is up to you @stsewd since you are the security master here 😄 and the one that has been working on this chunk of code.

stsewd · 2023-08-02T21:17:16Z

readthedocs/vcs_support/backends/git.py

-        return bool(self.submodules)
+        # TODO: remove when all projects are required
+        # to have a config file.
+        return any(self.submodules)


This results in 2 calls to git config for projects that don't have a config file.

readthedocs/vcs_support/backends/git.py

humitos

This looks good 👍🏼 . I have a few questions and suggestions to continue talking about.

However, the most important thing I want to ask here is "Have you tested this with GIT_CLONE_FETCH_CHECKOUT_PATTERN feature flag?". We have that enabled by default to all projects on production and we are not using the old pattern anymore.

humitos · 2023-08-03T10:48:25Z

readthedocs/doc_builder/environments.py

+        except (TypeError, AttributeError):
+            pass


It would be good to do log.warning here, so we can easily realize if there is a problem from NR.

I can add a log here, but we will probably see a spike in logs, since we weren't logging these before.

Do you think we have lot of command's outputs that are not possible to decode? That shouldn't be normal, I'd say.

readthedocs/doc_builder/environments.py

humitos · 2023-08-03T10:53:54Z

readthedocs/rtd_tests/tests/test_backend.py

+        valid, submodules = repo.validate_submodules(self.dummy_conf)
+        self.assertTrue(valid)
+        self.assertEqual(
+            list(submodules), ["foobar", "path with spaces", "another-submodule"]


Where does the foobar submodule comes from? I don't see it on the file written in disk.

It's defined in the submodule branch of the test git repo.

readthedocs/rtd_tests/tests/test_doc_building.py

humitos · 2023-08-03T10:57:37Z

readthedocs/vcs_support/backends/git.py

+@dataclass(slots=True)
+class GitSubmodule:
+    path: str
+    url: str


Do you have a preference here?

I don't remember why we were checking if the URL has a supported scheme. Maybe that will help me to have a more informed opinion 😄

readthedocs/vcs_support/backends/git.py

humitos · 2023-08-03T11:12:13Z

Oh, one thing that I missed here. You should remove gitpython from the dependencies.

stsewd · 2023-08-03T16:37:45Z

Oh, one thing that I missed here. You should remove gitpython from the dependencies.

We still use it in the code that gets tags and branches.

stsewd · 2023-08-03T17:13:25Z

Have you tested this with GIT_CLONE_FETCH_CHECKOUT_PATTERN feature flag?".

yeah, I did.

humitos · 2023-08-03T17:14:57Z

We still use it in the code that gets tags and branches.

I understood this PR was meant to use git command to get these as well and be able to remove gitpython.

stsewd · 2023-08-03T17:26:25Z

I understood this PR was meant to use git command to get these as well and be able to remove gitpython.

Looks like that's still under a feature flag/logic

readthedocs.org/readthedocs/projects/tasks/mixins.py

Lines 52 to 55 in 5f14e95

    
           use_lsremote = vcs_repository.supports_lsremote and ( 
        
               self.data.project.has_feature(Feature.GIT_CLONE_FETCH_CHECKOUT_PATTERN) 
        
               or not vcs_repository.repo_exists() 
        
           )

Looks like #8968 is removing that logic.

stsewd · 2023-08-08T21:42:35Z

cc @agjohnson in case you have some thoughts on https://github.com/readthedocs/readthedocs.org/pull/10594/files#r1282426806

humitos

I added two code comments. Other than that, we are ready to merge it I'd say. We can come back and refactor the other small pieces we are talking in the comments if we want to.

humitos · 2023-08-09T14:09:35Z

I'm merging this so I can move forward with other PRs that depend on this one.

We are always querying for submodules, even when the user explicitly listed the submodules to include. We also have the option to include all submodules, instead of listing each one of them, we can just omit them, git will automatically default to include all of them. Ref #10594 (comment)

stsewd added 6 commits August 1, 2023 20:18

Build: replace GitPython with git commands

ba7f986

Cleanup

77d3681

Fix tests

5ecfcc2

Black

39bf8b6

Format

b724f5b

Test

d8a3607

stsewd commented Aug 2, 2023

View reviewed changes

stsewd marked this pull request as ready for review August 2, 2023 21:21

stsewd requested a review from a team as a code owner August 2, 2023 21:21

stsewd requested a review from humitos August 2, 2023 21:21

auto-assign bot assigned stsewd Aug 2, 2023

humitos approved these changes Aug 3, 2023

View reviewed changes

humitos mentioned this pull request Aug 3, 2023

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

Merged

stsewd added 3 commits August 3, 2023 12:15

Updates from review

a6e7f01

Merge branch 'main' into replace-gitpython

475874b

Black

5f14e95

stsewd requested a review from humitos August 8, 2023 21:42

Merge branch 'main' into replace-gitpython

60b637a

humitos approved these changes Aug 9, 2023

View reviewed changes

Apply suggestions from code review

848f2be

humitos merged commit fd9559b into main Aug 9, 2023

humitos deleted the replace-gitpython branch August 9, 2023 14:09

stsewd mentioned this pull request Aug 15, 2023

Git: simplify submodule checks #10635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: replace GitPython with git commands #10594

Build: replace GitPython with git commands #10594

stsewd commented Aug 2, 2023 •

edited

Loading

stsewd Aug 2, 2023

stsewd Aug 2, 2023

stsewd Aug 2, 2023 •

edited

Loading

humitos Aug 3, 2023

stsewd Aug 3, 2023

agjohnson Aug 9, 2023

stsewd Aug 9, 2023

humitos Aug 9, 2023

agjohnson Aug 9, 2023

humitos Aug 10, 2023 •

edited

Loading

stsewd Aug 2, 2023

humitos left a comment

humitos Aug 3, 2023

stsewd Aug 3, 2023

humitos Aug 9, 2023

humitos Aug 3, 2023

stsewd Aug 3, 2023

humitos Aug 3, 2023

humitos commented Aug 3, 2023

stsewd commented Aug 3, 2023

stsewd commented Aug 3, 2023

humitos commented Aug 3, 2023

stsewd commented Aug 3, 2023

stsewd commented Aug 8, 2023

humitos left a comment

humitos commented Aug 9, 2023

Build: replace GitPython with git commands #10594

Build: replace GitPython with git commands #10594

Conversation

stsewd commented Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Aug 3, 2023

stsewd commented Aug 3, 2023

stsewd commented Aug 3, 2023

humitos commented Aug 3, 2023

stsewd commented Aug 3, 2023

stsewd commented Aug 8, 2023

humitos left a comment

Choose a reason for hiding this comment

humitos commented Aug 9, 2023

stsewd commented Aug 2, 2023 •

edited

Loading

stsewd Aug 2, 2023 •

edited

Loading

humitos Aug 10, 2023 •

edited

Loading