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

Automatic ruff fixes #655

Closed
wants to merge 2 commits into from
Closed

Automatic ruff fixes #655

wants to merge 2 commits into from

Conversation

tsx
Copy link
Contributor

@tsx tsx commented Feb 1, 2024

No functional changes, just ran ruff --fix ..

@tsx tsx requested a review from squeaky-pl February 1, 2024 08:09
@tsx tsx marked this pull request as ready for review February 1, 2024 08:10
@squeaky-pl
Copy link
Contributor

Can we get fixes without removing unused app imports as first step? Sync-engine is full of import side effects and my previous attempt at this ended up with broken syncing even the tests passed 😥.

@squeaky-pl squeaky-pl closed this Feb 5, 2024
@tsx
Copy link
Contributor Author

tsx commented Feb 5, 2024

This is sad. Sure, I can do one without import changes. Could you please point me to an example of an import side effect so I know what to look for?

@nsaje
Copy link
Contributor

nsaje commented Feb 5, 2024

@squeaky-pl did we find out why it broke the last time or just reverted?

@squeaky-pl
Copy link
Contributor

It was 2,5 years ago so I don't remember well. What I remember was that as soon as I shipped the change removing what it looks like not needed imports all the sync pods in all the clusters would start crash lopping because of some import related problem (something assuming that something else is imported earlier).

Admittedly this might be not the case anymore because the code changed substantially.

But to do this with confidence we would need a test that starts all the pod entry points and see if they started without problem.

In the meantime we could ship a version without removing those imports maybe?

This was referenced Feb 5, 2024
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.

3 participants