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

Make ChangePathFs compatible with new afero.Fs without updating #2216

Merged
merged 1 commit into from
Nov 3, 2021

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Nov 2, 2021

Given that we won't be using this method I don't think we need to have
actual real implementation for it which will require us to update
aforo.Fs, so I just returned an error.

This is predominantly motivated by the fact that an extension having a
dependency on a newer afero.Fs ( through another dependency for example)
will not compile as the interface afero.Fs was extended in v1.5.0.

Given that we won't be using this method I don't think we need to have
actual real implementation for it which will require us to update
aforo.Fs, so I just returned an error.

This is predominantly motivated by the fact that an extension having a
dependency on a newer afero.Fs ( through another dependency for example)
will not compile as the interface afero.Fs was extended in v1.5.0.
@github-actions github-actions bot requested review from na-- and yorugac November 2, 2021 10:50
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

hmm not ideal, but yeah, we don't require this and I doubt anyone is using k6 as a library to get ChangePathFs, 👍 😅

@na-- na-- requested review from imiric and codebien and removed request for yorugac November 2, 2021 12:28
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM

Maybe before merging @dgzlopes can confirm this works with his extension by checking out this branch and setting XK6_K6_REPO?

@mstoykov
Copy link
Contributor Author

mstoykov commented Nov 2, 2021

@imiric I already did, but I can wait for @dgzlopes to reconfirm as well :)

@na--
Copy link
Member

na-- commented Nov 2, 2021

:shipit:, it's obvious this will be a problem sooner or later, even without the specific extension, simply because afero decided to make a breaking change without releasing a new major version... 🤦‍♂️

@dgzlopes
Copy link
Member

dgzlopes commented Nov 2, 2021

I tested locally. It works like a charm!

Thanks for fixing this ❤️

@mstoykov mstoykov merged commit 98152ba into master Nov 3, 2021
@mstoykov mstoykov deleted the fixAferoCompatibility branch November 3, 2021 09:59
@na-- na-- added this to the v0.35.0 milestone Nov 3, 2021
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.

5 participants