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

Refactoring: Remove usage of deprecated LocalDirs field in SolveOpts #4583

Conversation

leandrosansilva
Copy link
Contributor

@leandrosansilva leandrosansilva commented Jan 24, 2024

As client.SolveOpt.LocalDirs is marked as deprecated, this PR moves all usage to LocalMounts instead.

Unfortunately the PR ended up quite long, but the changes are essentially mechanical.

I am afraid that this PR will also conflict with existing open PRs that touch tests, but I believe that resolving the conflicts will be trivial.

Once this PR is merged, client.SolveOpt.LocalDirs will be ready for removal, in case no external projects is using it.

@leandrosansilva
Copy link
Contributor Author

Note to the reviewer: I did not manage to get the integration tests to run reliably on my setup, so I hope having the tests running on github will be able to reveal issues.

To be honest, the tests don't run reliably for me even on master.

@leandrosansilva leandrosansilva marked this pull request as ready for review January 24, 2024 20:23
@leandrosansilva leandrosansilva force-pushed the refactoring/do_not_use_deprecated_local_dirs_in_tests branch from 2061a5d to 1e7461f Compare January 25, 2024 11:35
@leandrosansilva leandrosansilva changed the title Remove usage of deprecated LocalDirs field in SolveOpts Refactoring: Remove usage of deprecated LocalDirs field in SolveOpts Jan 25, 2024
Copy link
Member

@jedevc jedevc left a comment

Choose a reason for hiding this comment

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

Thanks @leandrosansilva 🎉

Could we have one usage of LocalDirs that checks that the deprecated property still works? Probably makes sense as a new test in client_test.go.

Otherwise looks pretty good to me (assuming tests pass 👀). One idea I remember having in #4094 was to use our StaticFS implementation in this PR to avoid needing to create temp directories at all:

for example, we could use our Static filesystem implementation in tests to prevent creating lots of temporary directories, or we could use our Merge filesystem implementation to allow easily creating variants of a single context.

If that seems interesting to you, feel free to try that out, but definitely not a blocker 🎉

Signed-off-by: Leandro Santiago <leandrosansilva@gmail.com>
Signed-off-by: Leandro Santiago <leandrosansilva@gmail.com>
@leandrosansilva leandrosansilva force-pushed the refactoring/do_not_use_deprecated_local_dirs_in_tests branch from 1e7461f to 5ffd907 Compare January 29, 2024 15:35
@leandrosansilva
Copy link
Contributor Author

Thanks @leandrosansilva 🎉

Could we have one usage of LocalDirs that checks that the deprecated property still works? Probably makes sense as a new test in client_test.go.

Good idea. Done.

Otherwise looks pretty good to me (assuming tests pass 👀). One idea I remember having in #4094 was to use our StaticFS implementation in this PR to avoid needing to create temp directories at all:

Interesting, I will have a look at that.

for example, we could use our Static filesystem implementation in tests to prevent creating lots of temporary directories, or we could use our Merge filesystem implementation to allow easily creating variants of a single context.

If that seems interesting to you, feel free to try that out, but definitely not a blocker 🎉

Great! One test got broken by my changes, but it's now fixed and pushed. Again, I could not get all the integration tests to pass locally, but I could fix the test that the previous execution on github pointed out.

@leandrosansilva
Copy link
Contributor Author

My apologies, I ended up committing some code I should not have committed. It's weird that running validate-lint locally did not find the issue. Fixing my mess now.

It will need to be removed in a follow up PR.

Signed-off-by: Leandro Santiago <leandrosansilva@gmail.com>
@leandrosansilva leandrosansilva force-pushed the refactoring/do_not_use_deprecated_local_dirs_in_tests branch from 5ffd907 to 704268a Compare January 29, 2024 16:47
@leandrosansilva
Copy link
Contributor Author

Hi @jedevc, FYI the PR is ready for review again, so please feel free to do so whenever suits you.

I will have a look at the StaticFS story, but I believe it's better done in a further PR.

@jedevc jedevc merged commit 7c0d261 into moby:master Feb 7, 2024
63 checks passed
@jedevc
Copy link
Member

jedevc commented Feb 7, 2024

Thanks @leandrosansilva! Much appreciated!

@leandrosansilva leandrosansilva deleted the refactoring/do_not_use_deprecated_local_dirs_in_tests branch February 7, 2024 15:20
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.

2 participants