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

Request: Update the examples directory #190

Closed
UebelAndre opened this issue Aug 1, 2020 · 7 comments
Closed

Request: Update the examples directory #190

UebelAndre opened this issue Aug 1, 2020 · 7 comments

Comments

@UebelAndre
Copy link
Collaborator

Every time smoke-test.sh is run, it creates huge amount of diffs in ./examples/* would it be possible to have these examples regenerated?

@samschlegel
Copy link
Contributor

samschlegel commented Aug 5, 2020

It's unclear to me why this directory exists. Is it so that people can see what the generated structure looks like without having to run it themselves? I think that makes sense to have.

It might make sense to have one of the tests in TravisCI be that no changes were made in examples when running smoke-test.sh. examples/WORKSPACE was updated manually and not in the smoke-test, so CI isn't using the changes that were made. Those changes were also invalid (bad sha256) so if someone tries to copy them they're going to have a bad time. It also makes it much more noisy and painful to add new smoke tests when the repo isn't up to date.

Possibly splitting the example gen out of smoke-test.sh so that not everything has to be built in order to generate would make this easier as well.

@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Aug 5, 2020

It's unclear to me why this directory exists. Is it so that people can see what the generated structure looks like without having to run it themselves? I think that makes sense to have.

Isn't ./examples the directory exactly this? A place where people can see what the generated structure looks like?

It might make sense to have one of the tests in TravisCI be that no changes were made in examples when running smoke-test.sh. examples/WORKSPACE was updated manually and not in the smoke-test, so CI isn't using the changes that were made.

I do feel like the state of ./smoke-test and ./example isn't that great right now and I feel like something could be done in in TravisCI to clean them up.

Those changes were also invalid (bad sha256) so if someone tries to copy them they're going to have a bad time.

What changes? The changes in #191 ?

The entire reason I requested this update was so that when I run smoketest I don't have a huge diff but additionally, it seems a lot has changed since ./examples was first added

@samschlegel
Copy link
Contributor

samschlegel commented Aug 17, 2020

Oh man I wrote up a whole response to this but it poofed 😞 Also my first response was late at night and in hindsight it reads a little loopy lol

Isn't ./examples the directory exactly this? A place where people can see what the generated structure looks like?

Yeah it is. I think the point I was trying to make was that it isn't always kept up to date, so it doesn't feel like it does that job very well

What changes? The changes in #191 ?

Ah no, I meant the changes that had been made to examples/WORKSPACE in e2cc36f . At least for me the SHA was wrong, so it didn't work if you just went into the examples directory and tried to build anything. It seems like it's been manually edited several times instead of reusing what the smoke test generates. I added an issue to track adding a check to CI #198

@UebelAndre
Copy link
Collaborator Author

Oh man I wrote up a whole response to this but it poofed 😞

Oh no, that sucks!

Also my first response was late at night and in hindsight it reads a little loopy lol

Haha no worries, thanks for your reply! 😄

I added an issue to track adding a check to CI #198

This sounds great to me. Though, that said, do you think #191 is still worth attempting to apply? Or do you think it'd be better to instead work on #198?

@samschlegel
Copy link
Contributor

#191 is going to be a requirement for merging #199 since without it, the test fails 😄

@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Aug 18, 2020

@samschlegel Ok, I updated the PR based on your feedback. Two notable changes added as well are that I renamed WORKSPACE to WORKSPACE.bazel and I added a .bazelversion file (which is generated by smoke-test.sh). Maybe one day this project can be built using Bazel, I think there's an issue ticket requesting that already.

edit: Found it - #61

@UebelAndre
Copy link
Collaborator Author

#191 has been merged thus satisfying this request.

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

No branches or pull requests

2 participants