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

Clone repo instead of init + fetch and take advantage of git-autoshare #55

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

joao-p-marques
Copy link
Contributor

Closes #45

As mentioned in the issue discussion, the aggregation process could be improved to start with a git clone instead of a init followed by fetch. This PR implements that.

That also opens the possibility to use git-autoshare to speed up the process and save space. This PR also adds that functionality, whilst keeping the original setup working when you don't have git-autoshare installed in your system.

@Tecnativa
TT32042

ping @sbidoul @yajo

git_aggregator/repo.py Outdated Show resolved Hide resolved
@sbidoul
Copy link
Member

sbidoul commented Oct 6, 2021

git-aggregator should not have to know about git-autoshare.

@joao-p-marques joao-p-marques force-pushed the use-git-autoshare branch 2 times, most recently from cf42778 to d99b6b9 Compare October 6, 2021 12:50
@sbidoul
Copy link
Member

sbidoul commented Oct 6, 2021

BTW if you are playing with this, you could try the --filter=blob:none git option to optimize download times, it might help a lot.

@joao-p-marques
Copy link
Contributor Author

git-aggregator should not have to know about git-autoshare.

Ok, but then how can it take advantage of git-autoshare? As we are launching git commands from Python, the suggested bash script would not work, I think...

@sbidoul
Copy link
Member

sbidoul commented Oct 6, 2021

If you do a subprocess.call(["git",...]) it should look in PATH, I think.

@joao-p-marques
Copy link
Contributor Author

BTW if you are playing with this, you could try the --filter=blob:none git option to optimize download times, it might help a lot.

Ok, I will set this do Draft while I fix the tests and look into this option... Thanks for the suggestions!

@yajo
Copy link
Contributor

yajo commented Oct 8, 2021

git-aggregator should not have to know about git-autoshare.

Do you mean that git-autoshare is used automatically on some git commands?

@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #55 (6e2ecdd) into master (1047659) will increase coverage by 0.77%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   81.44%   82.22%   +0.77%     
==========================================
  Files           6        6              
  Lines         388      405      +17     
==========================================
+ Hits          316      333      +17     
  Misses         72       72              
Impacted Files Coverage Δ
git_aggregator/repo.py 73.04% <100.00%> (+2.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1047659...6e2ecdd. Read the comment docs.

@joao-p-marques
Copy link
Contributor Author

CI should be fixed now...

I removed the git-autoshare part for now, and will try to work around it with the bash script as mentioned, but I still need some testing to find if it is the best solution...

repository,
target_dir,
)
cmd = ('git', 'clone')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmd = ('git', 'clone')
cmd = ('git', 'clone', '--filter=blob:none')

And maybe you don't even need git-autoshare.

Copy link
Contributor

Choose a reason for hiding this comment

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

I read the docs but don't understand. What does that do?

Copy link
Member

Choose a reason for hiding this comment

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

@yajo have a look at this article.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've been doing tests and indeed we're gonna use it. It's a great feature. However, it's also true that it doesn't fix all the problems we want to fix, so we'll use it in combination with git-autoshare.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this suggestion is applied, IMHO the PR would be complete, as the rest of using git-autoshare is doable with the wrapper script and/or can be done in a later PR.

So could we move forward with this? Thanks.

Choose a reason for hiding this comment

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

This is the only debatable thing. Should I add it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd do it.

Choose a reason for hiding this comment

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

OK, thanks.

@yajo
Copy link
Contributor

yajo commented Oct 19, 2021

Can't the git command wrapper be included in git-autoshare as an extension? So we pipx install git-autoshare[git-wrapper] and get it working out of the box?

@sbidoul
Copy link
Member

sbidoul commented Oct 19, 2021

Can't the git command wrapper be included in git-autoshare as an extension? So we pipx install git-autoshare[git-wrapper] and get it working out of the box?

Must think about it - probably not trivial. What is sure is that it must not be a python script because of python startup time. That's why there is a bash example.

@yajo
Copy link
Contributor

yajo commented Oct 19, 2021

I don't think python time is a real problem, TBH. After all, you're gonna use it for git-autoshare itself and it doesn't seem to be a problem, right? You're not cloning all day after all. And bash is less maintainable and portable.

@sbidoul
Copy link
Member

sbidoul commented Oct 19, 2021

I don't think python time is a real problem, TBH. After all, you're gonna use it for git-autoshare itself and it doesn't seem to be a problem, right? You're not cloning all day after all. And bash is less maintainable and portable.

Trust me, adding 100ms to the startup time of your git command is a problem. I've tried.

Copy link

@sebastienbeau sebastienbeau left a comment

Choose a reason for hiding this comment

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

LGTM Tested
@sbidoul let's merge it ?

@sbidoul
Copy link
Member

sbidoul commented Apr 6, 2022

The PR is still in draft state. @joao-p-marques is it ready ?

@yajo
Copy link
Contributor

yajo commented Apr 7, 2022

ping @Tardo

@sbidoul
Copy link
Member

sbidoul commented Sep 7, 2022

@joao-p-marques gentle nudge. Is this PR ready (it is still draft)?

@pedrobaeza
Copy link

Joao is no longer at Odoo world, so he can't say. Checking the PR, seems in good state and tested by several actors, so I put it as ready.

@pedrobaeza
Copy link

It seems I'm not able to change the PR status.

@sbidoul sbidoul merged commit 72c28c5 into acsone:master Sep 20, 2022
@sbidoul
Copy link
Member

sbidoul commented Sep 20, 2022

I added the --filter=blob:none option and merged manually.

@pedrobaeza pedrobaeza deleted the use-git-autoshare branch September 20, 2022 18:08
@pedrobaeza
Copy link

Thanks

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.

using local cache
5 participants