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

[13.0][REF] Porte da geração do certificado fake para o erpbrasil.assinatura #1844

Merged
merged 6 commits into from
Mar 31, 2022

Conversation

antoniospneto
Copy link
Contributor

backport do #1843 sem o commit 13a741c

substitui a pr #1777

depende do erpbrasil/erpbrasil.assinatura#32

@rvalyi
Copy link
Member

rvalyi commented Mar 22, 2022

@netosjb talvez vale a pena fazer o mesmo commit do que na 14.0, de repente isso ajuda algum port de transiçao da 12.0 para a 14.0

rvalyi
rvalyi previously approved these changes Mar 23, 2022
@rvalyi rvalyi dismissed their stale review March 23, 2022 00:22

Tests failing

@antoniospneto
Copy link
Contributor Author

A gente removeu o uso dos pacotes do OpenSSL na criação do certificado fake, mas esses pacotes ainda estão sendo usado na classe Certificado do erpbrasil.assinatura: https://github.com/erpbrasil/erpbrasil.assinatura/blob/2cf09fa29ac47a7ac052800f28a9133188fc2797/src/erpbrasil/assinatura/certificado.py#L9

acho que podemos tentar remover essa dependência de lá tbm, acredito que dá de fazer tudo usando o cryptography, em breve posso propor uma pr para essa alteração.

@rvalyi
Copy link
Member

rvalyi commented Mar 23, 2022

A gente removeu o uso dos pacotes do OpenSSL na criação do certificado fake, mas esses pacotes ainda estão sendo usado na classe Certificado do erpbrasil.assinatura: https://github.com/erpbrasil/erpbrasil.assinatura/blob/2cf09fa29ac47a7ac052800f28a9133188fc2797/src/erpbrasil/assinatura/certificado.py#L9

acho que podemos tentar remover essa dependência de lá tbm, acredito que dá de fazer tudo usando o cryptography, em breve posso propor uma pr para essa alteração.

Eu não tinha visto. É seria uma boa fazer esse refator...

@marcelsavegnago marcelsavegnago requested a review from rvalyi March 23, 2022 02:45
@marcelsavegnago
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 13.0-ocabot-merge-pr-1844-by-marcelsavegnago-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Mar 23, 2022
Signed-off-by marcelsavegnago
@marcelsavegnago
Copy link
Member

@rvalyi bobei aqui e mandei o merge sem ter passado nos testes do travis. Tem como abortar o merge ?

@rvalyi
Copy link
Member

rvalyi commented Mar 24, 2022

@netosjb vc consegue especificar a versão de um pacote no external_dependencies do manifest sim (vc apenas não consegue usar uma revisão/branch do repo git).
Veja esse examplo https://github.com/OCA/account-financial-tools/blob/14.0/account_loan/__manifest__.py#L22

@antoniospneto
Copy link
Contributor Author

@netosjb vc consegue especificar a versão de um pacote no external_dependencies do manifest sim (vc apenas não consegue usar uma revisão/branch do repo git). Veja esse examplo https://github.com/OCA/account-financial-tools/blob/14.0/account_loan/__manifest__.py#L22

ahh ok, atualizei a pr para fazer um teste, vamos ver se assim o trevis passa.

@rvalyi
Copy link
Member

rvalyi commented Mar 24, 2022

outra coisa, é trAvis que se chama...

@mbcosta
Copy link
Contributor

mbcosta commented Mar 24, 2022

@netosjb parece que ocorreu erro do pre-commit, vc pode rodar localmente na pasta do projeto esse comando

pre-commit run --all --show-diff-on-failure --verbose --color always

que ele vai alterar automaticamente os arquivos para o padrão esperado, assim não gera esse erro aqui

@rvalyi
Copy link
Member

rvalyi commented Mar 24, 2022

@mbcosta @netosjb na vdd o melhor é fazer pre-commit install assim o hook é ativado e vc so consegue fazer seu commit se passa.

@antoniospneto
Copy link
Contributor Author

@netosjb parece que ocorreu erro do pre-commit, vc pode rodar localmente na pasta do projeto esse comando

pre-commit run --all --show-diff-on-failure --verbose --color always

que ele vai alterar automaticamente os arquivos para o padrão esperado, assim não gera esse erro aqui

É que eu editei pelo navegador 😅 logo vou corrigir, é que estou sem o meu notebook agora

@mbcosta
Copy link
Contributor

mbcosta commented Mar 24, 2022

@rvalyi não sei se entendi bem, esse comando que vc passou apenas retorna:
$ pre-commit install
pre-commit installed at .git/hooks/pre-commit

enquanto que ao rodar o mesmo comando do Travis https://github.com/OCA/l10n-brazil/blob/13.0/.travis.yml#L31 localmente
$ pre-commit run --all --show-diff-on-failure --verbose --color always

ocorre a alteração dos arquivos de forma automática, ou faltou algo?

@netosjb tranquilo, escrevi apenas por ser algo simples de resolver, caso você não soubesse, porque quando não passa o pre-commit os outros testes nem chegam a rodar

@rvalyi
Copy link
Member

rvalyi commented Mar 24, 2022

pre-commit install

depois disso qualquer novo commit vai tentar rodar o pre-commit. Então vc simplesmente não consegue mandar mais commit novo sem o pre-commit ter passado. Mas isso nao vai checar algo no passado tb não.
Enfim é a forma normal de usar. Eu acho valido desativar apenas nas fases mais punks de migração de módulos super complexos.

@mbcosta
Copy link
Contributor

mbcosta commented Mar 24, 2022

certo @rvalyi agora entendi, obrigado

@mbcosta
Copy link
Contributor

mbcosta commented Mar 24, 2022

@netosjb parece que está ocorrendo uma instabilidade na inicialização do Travis, eu vi isso hoje no PR #1846 e o que fiz para contornar o problema foi um commit vazio para conseguir fazer o force-update:
$ git commit --amend
$ git push Remote Branch:Branch -f

e acabou rodando, aconteceu duas vez lá, e parece que ocorreu agora aqui, se puder veja se fazendo isso o Travis inicia

cc @rvalyi @renatonlima @marcelsavegnago

@antoniospneto
Copy link
Contributor Author

@mbcosta valeu , acho que agora vai

@marcelsavegnago
Copy link
Member

l10n_br_fiscal/tools/misc.py:8: [W7936(missing-manifest-dependency), ] Missing external dependency "erpbrasil.assinatura" from manifest.

@rvalyi
Copy link
Member

rvalyi commented Mar 24, 2022

l10n_br_fiscal/tools/misc.py:8: [W7936(missing-manifest-dependency), ] Missing external dependency "erpbrasil.assinatura" from manifest.

tem que ver se teria como fazer algo na configuraçao do pacote pro namespace da importaçao nao mudar apesar do nome do pacote no pypi mudar. Tenho quase certeza que é possivel.

@antoniospneto
Copy link
Contributor Author

antoniospneto commented Mar 31, 2022

Pessoal o problema é o conflito da versão das libs, eu tentei forçar a instalação do cryptography 35.0 mas não rolou, vou desfazer o meu ultimo commit.

o pyopenssl 19.0 não funciona com cryptography 36.0 ! para resolver o conflito teria que subir a versão do pyopenssl para a 22.0 ou descer a versão do cryptography para a 35.0.

Mas essa incompatibilidade só tá estourando erro ao tentar executar essa linha do erpbrasil.assinatura:
File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/erpbrasil/assinatura/certificado.py", line 60, in __init__ if raise_expirado and self._x509.has_expired():

esse método has_expired() que está sendo usado na linha acima faz parte do módulo crypto do PyOpenSSL.

Por isso eu abri uma PR no erpbrasil.assinatura para remover o uso do módulo crypto do PyOpenSSL: erpbrasil/erpbrasil.assinatura#33

Acredito que com isso podemos resolver o problema.

@rvalyi você consegue subir no pypi a lib erpbrasil.assinatura-nopyopenssl com o merge desta nova pr ?

@rvalyi
Copy link
Member

rvalyi commented Mar 31, 2022

eu abri uma PR no erpbrasil.assinatura

boa @netosjb . Seu trabalho nessa limpeza esta sendo muito apreciado! Vou jantar algo e daqui pouco eu faço um release do erpbrasil.assinatura-nopyopenssl.

@rvalyi
Copy link
Member

rvalyi commented Mar 31, 2022

@netosjb eu publiquei a v1.5.0 com seu PR https://pypi.org/project/erpbrasil.assinatura-nopyopenssl/1.5.0/
re-iniciei o Travis aqui e os testes passaram! Mas o que não passou foi o pre-commit.
Abri esse PR para resolver isso: Engenere#9
Vc consegue fazer o merge? Que ai eu accredito que isso resolve tudo. Depois seria apenas adaptar para a v14 tb. Parabens pelo trabalho!

cc @felipemotter @renatonlima @marcelsavegnago @mbcosta

@rvalyi
Copy link
Member

rvalyi commented Mar 31, 2022

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-1844-by-rvalyi-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 8545828 into OCA:13.0 Mar 31, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at ccabd93. Thanks a lot for contributing to OCA. ❤️

@antoniospneto
Copy link
Contributor Author

Obrigado @rvalyi

@rvalyi
Copy link
Member

rvalyi commented Apr 1, 2022

Obrigado @rvalyi

obrigado vc @netosjb vamos que vamos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants