search

OCA / oca-addons-repo-template

OCA Repository Template
MIT License
61 stars 89 forks source link

pre-commits generates pandoc-*.deb when converting rst to markdown #262

Closed sbidoul closed 2 months ago

sbidoul commented 3 months ago

See for example this commit: https://github.com/OCA/l10n-spain/commit/6ab2763cbef17f2a81528e7564ddf36ce870ad79#commitcomment-143043458

Circumstances when this happens need to be clarified. It may come from the readme generator when it converts markdown back to rst, when pandoc is not yet installed on the machine.

pedrobaeza commented 3 months ago

Someone can make a CI check like the dependencies one that goes to red when a .deb is present? Or give me any pointer on how to do it. We will need later a mass update on 17.0 branches.

Another thing is to remove completely these files, so the only solution is to forced push a clean branch without this. Do you agree?

sbidoul commented 3 months ago

Pre-commit has a way to fail on specific files: https://pre-commit.com/#fail We coud add that to the template.

Another thing is to remove completely these files, so the only solution is to forced push a clean branch without this. Do you agree?

No, please, never ever force-push on main branches. That will cause all sorts of problems.

pedrobaeza commented 3 months ago

We can't deal with 30 MB of diff adding it and another 30 MB of diff removing them, and if you dare to do git pull in any branch, you will have the same size increasing. That's a lot of overhead and provoking unneeded waste of resources.

The systems should fetch origin branch + reset --hard origin/branch for a secure auto-update. It's how we do it.

carmenbianca commented 3 months ago

I agree that the cumulative waste of resources merits a force push, even if it's unfortunate and inconvenient.

pedrobaeza commented 3 months ago

It's even worse now in some repos, like OCA/social, where there's 2 .deb with different versions: https://github.com/OCA/social/tree/17.0, so that 60 + 60 = 120 MBs

pedrobaeza commented 3 months ago

As a little alleviation, I have added this to the migration documentation:

https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-17.0/_compare/4c480a672acf063a72b568879c90bc04d0bf3abe...1040301847c99d82ada874a94e9b8f14b9221757

gurneyalex commented 3 months ago

Could we have a global *.deb line in the template gitignore as a mitigation measure?

pedrobaeza commented 3 months ago

Yeah, that can be another option indeed...

gurneyalex commented 3 months ago

https://github.com/OCA/oca-addons-repo-template/pull/263

But not all repos will be synced in time so having this in the migration instruction is still useful.

sbidoul commented 3 months ago

Thanks @gurneyalex. Hopefully someone can spare the time to look at the root cause.

sbidoul commented 3 months ago

The fix has landed https://github.com/OCA/maintainer-tools/pull/620. Thanks @nilshamerlinck, who also suggests the procedure to remove and force-push in https://github.com/OCA/maintainer-tools/issues/619#issuecomment-2208259936

Now we need to update this template with the last maintainer tools commit. I'll then take care to push the template update, at least to 17.0 branches.

Regarding force push, this will make some commits on the affected main branches unreachable, so this could also cause troubles for folks who pin OCA commits directly.

pedrobaeza commented 3 months ago

@sbidoul are you going to do the .deb cleaning?

sbidoul commented 3 months ago

I'm not convinced we should do it, but I'll not block. I do think this should communicated clearly and in advance, though, as this is disruptive to many workflows.

pedrobaeza commented 3 months ago

OK, I will do it. I know it's not the best, but a lot of people do git pull freely in any local branch of any version (which fetchs everything from the origin remote), and wasting that amount of downloaded MBs is too much IMO.

sbidoul commented 2 months ago

The latest repo template is being pushed to 17.0 branches.

pedrobaeza commented 1 month ago

I have found that there's also another package format: .pkg. Please include it merging https://github.com/OCA/oca-addons-repo-template/pull/269 and releasing a new template. It seems this is also a problem in 16.0: https://github.com/OCA/delivery-carrier/pull/864, so please deploy it in all the 16.0 branches.

ap-wtioit commented 1 week ago

@pedrobaeza here is a migration pull request that still had the .deb files https://github.com/OCA/server-auth/pull/694, the rm step is no longer in the documentation for migration. Was this a user error or is it still happening for migrations to 17.0?

pedrobaeza commented 1 week ago

I removed it because it was causing another side effect: the repository that already contained some .deb added the removal diff to the migration PR...

Theoretically, the fix was applied for not happening again. @sbidoul did you update all the repos templates?