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

Use a wrapper for test-patch to enable standalone mode #195

Merged
merged 1 commit into from
Aug 30, 2019

Conversation

aw-was-here
Copy link
Contributor

No description provided.

@deitch
Copy link
Contributor

deitch commented Aug 15, 2019

Is there no way to run the entire thing inside a docker container like I tried to do in the other PR @aw-was-here ? The original one did s lot inside CircleCI, but that was at least in a container. This extracts a lot of it from circle (good), but runs a lot locally (not as good). Can we wrap it all up in a container so all I need to execute is some docker run command? No other downloads and dependencies?

@aw-was-here
Copy link
Contributor Author

Is there no way to run the entire thing inside a docker container like I tried to do in the other PR @aw-was-here ? The original one did s lot inside CircleCI, but that was at least in a container. This extracts a lot of it from circle (good), but runs a lot locally (not as good). Can we wrap it all up in a container so all I need to execute is some docker run command? No other downloads and dependencies?

Could you explain what your concerns are?

@aw-was-here aw-was-here force-pushed the aw-standalone-test-patch branch 2 times, most recently from 72e3869 to ed9299c Compare August 15, 2019 20:33
@aw-was-here
Copy link
Contributor Author

aw-was-here commented Aug 15, 2019

So a couple of gotchas with the current codebase:

  • For local runs, the Yetus download wrapper only fetches released versions. We're currently using master (since that has the Go support), but 0.11.0 is just about to start the release process. Installing a local version of master and pointing YETUS_HOME at it will use that version instead. This should be a short-term problem.
  • During testing of this patch, I hit YETUS-904. junit report doesn't work properly under --docker apache/yetus#72 . Again, this only impacts local runs. A temporary work-around would be to only configure the junit report during Circle CI runs. My hope is that this will make 0.11.0 so we won't have to use a workaround.
  • The version of Apache Yetus to use isn't defined in a single place. There are a few potential ways to fix that, with the best probably being for Apache Yetus to have a way to define build args such that the Dockerfile could use that as input.
  • Some of the directories are a bit hard-coded (e.g., /tmp/yetus-out) due to Circle CI's needs. With a bit more work, it might be possible to make that more flexible.

@deitch
Copy link
Contributor

deitch commented Aug 16, 2019

Could you explain what your concerns are?

Sure. A number of them

  1. Downloading locally means placing a file (or files) either on a machine or in the git repo.
  2. Downloading may mean downloading each time. Docker does an excellent job seeing that I already have the image and not downloading again. This is a primary concern for those with bandwidth constraints (which is what drove this thread in the first place).
  3. Downloading and running locally means dependencies. Can you guarantee my version of bash? Utilities (think BSD vs Linux vs MacOS grep or other utilities)? The whole beauty of OCI images is that they basically solve this.

It basically comes down to, why would I want to run something locally, to launch docker, to run something, when I could have it all packaged up in a single image? We have a basic set of local requirements (docker, make), but other than that, everything is in a container.

Does that explain?

@aw-was-here
Copy link
Contributor Author

Could you explain what your concerns are?

Sure. A number of them

1. Downloading locally means placing a file (or files) either on a machine or in the git repo.

I'm not sure I understand the concern given that EVE is primarily being done in Go, which not only downloads components but also updates the source repo as part of the build. Then there is the cost of that initial docker image downloads ...

2. Downloading may mean downloading each time. Docker does an excellent job seeing that I already have the image and not downloading again. This is a primary concern for those with bandwidth constraints (which is what drove this thread in the first place).

The yetus-dl wrapper downloads ~150k and unpacks it into a Well Known Location to cache it between runs. Additionally, if this is a concern, then installing it locally and using that location is also an option. (That's how I tested yetus master.)

3. Downloading and running locally means dependencies. Can you guarantee my version of bash? Utilities (think BSD vs Linux vs MacOS grep or other utilities)? 

With a lot of confidence, actually. The project is extremely careful about that. [e.g., the only known cross-platform bug that I know of is https://issues.apache.org/jira/browse/YETUS-810 .]

The whole beauty of OCI images is that they basically solve this.

Well... sort of. Things start to fall apart when dealing with volume mounts (which is part of the reason why #183 doesn't really work).

It basically comes down to, why would I want to run something locally, to launch docker, to run something, when I could have it all packaged up in a single image?

That's sort of a false equivalency; there's always going to be wrapper code involved to make the proper switch between non-Circle CI and Circle CI. It's practically unavoidable.

We have a basic set of local requirements (docker, make), but other than that, everything is in a container.

That's not really true either, but that's ok. But I will point out that there's nothing preventing anyone from putting all of Yetus and the projects testing requirements into the same development Dockerfile. Then that would meet your goal. But I don't think people will be willing to wait 45 minutes for it to build ...

Does that explain?

Sure. I just disagree.

@deitch
Copy link
Contributor

deitch commented Aug 16, 2019

there's nothing preventing anyone from putting all of Yetus and the projects testing requirements into the same development Dockerfile

That's what I tried to do with #185, just as a separate image rather than the developer one, so they can iterate on different cycles. Not sure why it failed, in the end, though.

Sure. I just disagree

No argument on that one. :-)

In the end, it doesn't matter much, as long as we get it so that the people who want to run yetus locally (i.e. pre-opening a PR to see the differences, can do so). The rest is tech implementation debates, which matter only so much. Happy to see any way that it works.

@aw-was-here
Copy link
Contributor Author

Apache Yetus 0.11.0 was released last night. This PR should be good to go now.

@aw-was-here aw-was-here changed the title WIP: use a wrapper for test-patch to enable standalone mode Use a wrapper for test-patch to enable standalone mode Aug 30, 2019
* Move from Apache Yetus master to 0.11.0
* Provide a 'make yetus' target, moving most of the Apache Yetus setup into shared resources
* Provide the necessary plumbing to either use an installed version of Apache Yetus (e.g., Circle CI) or download one on demand
* Upgrade the golangci-lint binary to a released version

Signed-off-by: Allen Wittenauer <aw@effectivemachines.com>
Copy link
Contributor

@rvs rvs left a comment

Choose a reason for hiding this comment

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

LGTM. Committed.

@rvs rvs merged commit 411cfdc into lf-edge:master Aug 30, 2019
@aw-was-here aw-was-here deleted the aw-standalone-test-patch branch August 30, 2019 20:32
@deitch
Copy link
Contributor

deitch commented Aug 31, 2019

Thanks!

@deitch
Copy link
Contributor

deitch commented Aug 31, 2019

@kalyan-nidumolu can you verify that make yetus gives you what you need running locally?

milan-zededa pushed a commit to milan-zededa/eve that referenced this pull request Nov 7, 2023
…ogging

Only log containerd and k3s pids when they change, cutting down logging
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