METRON-1344: Externalize the infrastructural components using integration tests #858
Conversation
|
@merrimanr That must be it, I didn't do a new pull. Now, to figure out how to do it with the checkout-pr script and not have to start over. |
|
OK, I think what you have done to this point is great @merrimanr, really great work. I like the speed of getting compose up and everything. The idea of doing the namespaces for ES is great too. I did not run the integration test To summarize my feedback at this point:
docker-compose pscan do it minimally. |
|
@merrimanr would you imagine someone just leaving this running all the time? or starting on demand per use? |
|
Point taken on maintaining 2 different solutions. Which services would people want to enable remote debugging on? I've only ever debugged Metron code so I don't know which services are important to people or why you would need to put breakpoints in core service code. Enabling remote debugging will likely be a little bit different for each service since each project has their own mechanism for adding extra jvm params. I'm happy to do a POC for this once I understand which services are important. @ottobackwards I usually just leave mine running but spinning it up/down is really fast once all the images are loaded. |
|
We would want to remote debug any code that we maintain in Metron, so any services which run metron code would need to have remote debugging (e.g. storm, the REST app). Ironically, this is one of those situations where just running the in-memory components in a separate process would be easier, since it's just a JVM param to pass in to enable remote debugging. |
|
If we think that remote debugging would cause this to balloon, should we consider making this a feature branch so multiple PRs with iterative solutions can exist and be reviewed separately? Just a thought. |
|
As long as breakpoints are limited to code we maintain in Metron, I think it's just a matter of documenting how to set it up. For example, I run REST locally in Intellij and debugging works fine against the in memory components or full dev. I think the Storm topologies should continue to be run locally with LocalCluster so we're covered there too as along as the topologies can access the services running in Docker. The only difference I can think of would be that now you can't put a breakpoint in Kafka or Elasticsearch server side code without enabling remote debugging. I have no problem with this being a feature branch. There is a lot of work to be done and design issues to work through. The tradeoff of having this in a feature branch is that it will delay the ability to run the UI e2e tests in our build. The tradeoff of doing it incrementally is that our build times will increase until we switch the tests over because we will have to use both in memory components and Docker. I'm happy to do it either way. |
|
This smells like it might be a bigger effort than one PR, I think I'd rather have smaller PRs on a feature branch so each is easier to review. I would expec that we will want to try it out and live with it a bit as it will impact day-to-day productivity fairly fundamentally. I'd vote for a feature branch, but I'd like to hear other people's impressions. |
merrimanr
changed the base branch from
master
to
feature/METRON-1344-test-infrastructure
|
I created a feature branch called "feature/METRON-1344-test-infrastructure" and switched the base of this PR to that instead of master. What is the next step? |
|
I would be for +1 and landing this pr on the feature branch, then we can discuss next steps, proposals and start new pr's against FB |
|
I think getting the feature branch set up was a great first step, so thanks for setting that up, @merrimanr. I agree with @ottobackwards, that we need a discussion on what the next steps are. For me, I think it's getting reqs fleshed out and agreed on (i.e. what's the end state where we pull in the branch, and what's follow on). I think getting a clear idea of end state for this phase is going to help avoid feature creep and the branch being stagnant. |
cestella
left a comment
cestella
left a comment
There was a problem hiding this comment.
This looks like a good first pass to go into the feature branch. It'd be good to have a clear picture of the end-state being proposed and the path to get there. I think it may be appropriate to have this in the form of a JIRA for infrastructure externalization with sub-JIRAs detailing the path forward. Thoughts?
| 'ip_dst_addr', 'host', 'alert_status', 'guid' ]; | ||
|
|
||
| page.clearLocalStorage(); | ||
| //page.clearLocalStorage(); |
There was a problem hiding this comment.
Can we remove this line instead of commenting it out?
| expect(page.getTableColumnNames()).toContain('Country', 'for renamed column names for alert list table'); | ||
|
|
||
| }); | ||
| // it('should rename columns from table configuration', () => { |
| specs: [ | ||
| './e2e/login/login.e2e-spec.ts', | ||
| './e2e/alerts-list/alerts-list.e2e-spec.ts', | ||
| // './e2e/login/login.e2e-spec.ts', |
There was a problem hiding this comment.
Similar comment to the above about removing vs commenting out code.
|
Maybe a Jira for the feature branch, and each PR as a subtask? |
|
Yep, I think a JIRA for the feature branch and each PR as a subtask is a good idea. |
|
@merrimanr Is it worth having a new discuss thread where you lay out what you've done, where you expect this to end, and what (if any) work that would be nice to have but isn't essential for this to be feature complete? i.e. is the endpoint where we merge to master (and I'm injecting my own bias here, so feel free to adjust as you see fit):
@ottobackwards @cestella Is that reasonable? I don't really want to make decisions regarding the specific end criteria of a feature branch in the POC PR. Sidenote, I like the top level PR and the child tasks for this. It'd be a good way to both organize and flesh out the TODO list you had and give everyone an easy way to see where things are (and where they can hop in and help out) |
|
I agree with the discuss thread, I'll leave comment on the list for there |
|
I agree with everything that has been said. I will address the minor changes @cestella suggested and start the discussion thread. Will METRON-1344 work as the base Jira for the feature branch? |
|
I'd actually recommend not using METRON-1344 as the base JIRA. I'd recommend creating a new JIRA where the requirements for the end-state are laid out in the description. I'd call this one a PoC JIRA that the base JIRA depends on. |
|
@merrimanr, I have created labels in jira that are applicable to feature branch Jiras, I would suggest creating a page for this feature branch were we can document things in this area, and using the labels on the liras metron-feature-canidate, {create a metron-feature-externalize-? label} |
|
@cestella are you good with merging this in to the feature branch? I believe I addressed your comments. |
|
@merrimanr I am +1 on getting this down to the feature branch and moving on. Let's get this moving |
|
Re-upping what I said before |
4 participants
Contributor Comments
This PR will add infrastructure to our Travis build that will allow the Alerts UI e2e test to be run. There are several outstanding issues that still need to be worked out so DO NOT MERGE this yet.
This is a first pass at a potential Docker-based solution and is meant to be a POC. The intention is to facilitate further discussion around the general approach and provide a working example that we can build off of.
A good place to start is the .travis file. This provides a good guide on how this infrastructure is spun up. It is assumed Docker and Docker Compose are installed. To use outside of travis in a local dev environment (assuming Mac OSX):
mvn clean install -DskipTestsmetron-contrib/metron-docker/scripts/create-docker-machine.sheval $(docker-machine env metron-machine)docker build ./metron-centos/ -t "metron-centos"cd metron-contrib/metron-docker-e2e/compose && docker-compose up -dA working environment should now be available. To verify get the Docker machine address with
echo $DOCKER_HOST. Services should be available on the ports specified inmetron-contrib/metron-docker-e2e/compose/docker-compose.yml. For example, assuming my $DOCKER_HOST is "tcp://192.168.99.100:2376", Elasticsearch should be available at http://192.168.99.100:9210.At this point only a single e2e test is run due to the significant refactoring being done in #857.
Here are my thoughts so far based on work in this PR:
There is still significant work to be done including:
Looking forward to some feedback.
Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
For all changes:
For code changes:
Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
For documentation related changes:
Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via
site-book/target/site/index.html:Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.