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.

Sign up for GitHub

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

Jump to bottom

refactor: Remove the need for the flux_test_gen files #4729

Merged
merged 1 commit into from
May 12, 2022
Merged

refactor: Remove the need for the flux_test_gen files #4729

merged 1 commit into from
May 12, 2022

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented May 10, 2022 •
edited
Loading

Take two of #4715 . Retains the flux_test_gen.go files so that go mod vendor does not omit the .flux files but they are now just empty dummy files. Then this exposes the test gathering such that packages that vendor us can just give the path to the stdlib directory inside their vendored flux dependency.

@Marwes Marwes requested a review from a team as a code owner May 10, 2022 14:24
@Marwes Marwes requested review from skartikey and removed request for a team May 10, 2022 14:24
@Marwes Marwes force-pushed the remove_flux_gen branch from 77141a2 to 6669791 Compare May 10, 2022 14:24
@skartikey
Copy link
Contributor

It looks good to me, but may I ask @wolffcm to look at it as well as the previous commit was reverted recently.

wolffcm
wolffcm reviewed May 12, 2022
View reviewed changes
Copy link

@wolffcm wolffcm left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, just a couple questions:

  1. What is it about having the _test_gen.go files that causes go mod vendor to retain all the .flux files?
  2. Did you test this out with your branch in the downstream repo?

@Marwes
Copy link
Contributor Author

Marwes commented May 12, 2022

What is it about having the _test_gen.go files that causes go mod vendor to retain all the .flux files?

Go seems to deliberately exclude non-package directories, so if there isn't a go file in a directory it is simply excluded golang/go#26366 .

Did you test this out with your branch in the downstream repo?

Yep, according to your instructions.

@wolffcm
Copy link

wolffcm commented May 12, 2022

Thanks for taking this on, it will be a big quality of life improvement.

@Marwes Marwes force-pushed the remove_flux_gen branch from 6669791 to 2af280a Compare May 12, 2022 16:31
@Marwes Marwes merged commit bad49e3 into master May 12, 2022
@Marwes Marwes deleted the remove_flux_gen branch May 12, 2022 18:12
jsternberg added a commit that referenced this pull request May 16, 2022
@jsternberg
revert: "refactor: Remove the need for the flux_test_gen files (#4729)"
e84dafa 
This reverts commit bad49e3.

This commit is attempting to remove the flux files to ast generation.
The problem with this is that downstream projects use this list of tests
to run end to end tests so it's not that simple to just remove these
files for those legacy tests.
jsternberg added a commit that referenced this pull request May 16, 2022
@jsternberg
revert: "refactor: Remove the need for the flux_test_gen files (#4729)…
dbeae19 
…" (#4760)

This reverts commit bad49e3.

This commit is attempting to remove the flux files to ast generation.
The problem with this is that downstream projects use this list of tests
to run end to end tests so it's not that simple to just remove these
files for those legacy tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wolffcm wolffcm wolffcm approved these changes

@skartikey skartikey Awaiting requested review from skartikey skartikey was automatically assigned from influxdata/flux-team

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Marwes @skartikey @wolffcm