8000 ci: fix integration tests errors with `text file busy` by jeronimoalbi · Pull Request #2674 · ignite/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

ci: fix integration tests errors with text file busy #2674

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

Merged
merged 7 commits into from
Aug 4, 2022

Conversation

jeronimoalbi
Copy link
Member

Related to #2657

The script has a fixed name to comply with `protoc` plugin naming
requirements but it it always generated in the same temporary folder
which means that concurrent calls would overwrite the `protoc-gen-ts_proto`
file.

This changeset generates the script in random temporary folders.

This fix might help correcting "flaky" integration tests.
8000
@jeronimoalbi jeronimoalbi self-assigned this Jul 28, 2022
@jeronimoalbi jeronimoalbi changed the title CI: fix integration tests errors with text file busy ci: fix integration tests errors with text file busy Jul 28, 2022
tbruyelle
tbruyelle previously approved these changes Jul 28, 2022
Copy link
Contributor
@tbruyelle tbruyelle left a comment

Choose a reason for hiding this comment

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

Thanks for the hard investigation!

@ilgooz
Copy link
Member
ilgooz commented Jul 29, 2022

👏

@lumtis
Copy link
Contributor
lumtis commented Jul 30, 2022

Is it still a draft @jeronimoalbi ?

@jeronimoalbi
Copy link
Member Author

Is it still a draft @jeronimoalbi ?

Yes, I don't think the current change really fixes the issue.
I wanted to discuss it.

This allow calling Generate multiple times using a single protoc binary.
Otherwise a protoc binary would be created each time Generate is called.
@jeronimoalbi
Copy link
Member Author
jeronimoalbi commented Aug 2, 2022

Yes, I don't think the current change really fixes the issue.
I wanted to discuss it.

New changes were added to use a single protoc binary during javascript code generation. These changes might fix the issue, but it is not really possible to check it because I can't reliably reproduce the "flaky" issue.

@jeronimoalbi jeronimoalbi marked this pull request as ready for review August 2, 2022 15:43
aljo242
aljo242 previously approved these changes Aug 3, 2022
Copy link
Contributor
@aljo242 aljo242 left a comment

Choose a reason for hiding this comment

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

LGTM. I think this is a quality improvement to the code regardless of if it fixes the issue :)

Copy link
Member
@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

Hey, looks like some conflicts are appeared.

@lumtis lumtis merged commit cd60592 into develop Aug 4, 2022
@lumtis lumtis deleted the fix/flaky-integration-tests branch August 4, 2022 08:16
Jchicode pushed a commit to Jchicode/cli that referenced this pull request Aug 9, 2023
* fix: change `protoc-gen-ts_proto` to generate in random folders

The script has a fixed name to comply with `protoc` plugin naming
requirements but it it always generated in the same temporary folder
which means that concurrent calls would overwrite the `protoc-gen-ts_proto`
file.

This changeset generates the script in random temporary folders.

This fix might help correcting "flaky" integration tests.

* refactor(pkg/protoc): add WithCommand option to Generate

This allow calling Generate multiple times using a single protoc binary.
Otherwise a protoc binary would be created each time Generate is called.

* refactor(pkg/cosmosgen): change JS generator to reuse protoc binary

* chore: simplify code

Co-authored-by: Alex Johnson <alex@shmeeload.xyz>
Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
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.

5 participants
0