Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

METRON-1239 Drop extra dev environments #852

Closed
wants to merge 3 commits into from

Conversation

nickwallen
Copy link
Contributor

@nickwallen nickwallen commented Nov 28, 2017

Per the previous discussion referenced in JIRA, this PR drops support for the Quick Dev and Code Lab environments. These have been effectively dead for quite a long time.

I tested this by performing a Full Dev deployment and Amazon EC2 deployment just to make sure I didn't delete anything that I should not have.

I'd really like to get the metron-deployments directory cleaned-up and reorganized to make it easier for new users to build and deploy Metron. This is the first step.

For all changes:

  • Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
  • Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • 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?

@ottobackwards
Copy link
Contributor

I think you missed the inventory/quick-dev-platform stuff

@nickwallen
Copy link
Contributor Author

Thanks @ottobackwards . I just pushed that fix, plus some other little things I found.

@ottobackwards
Copy link
Contributor

+1 by inspection

@anandsubbu
Copy link
Contributor

Hi @nickwallen , I found one more reference to quick dev - here.

Also, I could see that the contents within site/current-book needs an update, since it does not have the changes. Does mvn site need to be run in order to fix this?

@nickwallen
Copy link
Contributor Author

@anandsubbu: I found one more reference to quick dev - here.

I was confused about that README.vm file. Any idea what that is for?

@anandsubbu: Also, I could see that the contents within site/current-book needs an update, since it does not have the changes.

I left everything under site/current-book untouched because I thought we only change that on a release. But I could be wrong about that.

@anandsubbu
Copy link
Contributor

I was confused about that README.vm file. Any idea what that is for?

I am not entirely sure, @nickwallen . I could find a lot of similarities between this README.vm and the main Metron REST README.md though.
https://github.com/nickwallen/metron/blob/7145b06fff9f4ecc8e63d9cdd56ed955a49e676d/metron-interface/metron-rest/README.md

@justinleet
Copy link
Contributor

Glancing briefly, it looks like ReadMeUtils uses it as a template for the metron-rest README.md. Just running the main in there overwrites the metron-rest README.md. Which seems very odd, given that ReadMeUtils is in the test package.

There seems to be no documentation of this class, or its purpose, and I didn't dig enough into the code to figure it out. Even not knowing the details and assuming I'm not misreading what's happening, I don't like that there's an expectation of editing a README.vm file, then running a program to produce the final output README.md. README.md can vary independently of README.vm. And it already has.

It's outside the scope of this ticket, but at minimum, that class needs to be moved out of test, it needs to be actually documented what the purpose of it is, the steps to use it, etc. Right now, though, unless someone comes up with a compelling reason not to, I'm in favor of killing it entirely. I don't ever see that being properly managed, even if it does have some utility built in.

@justinleet
Copy link
Contributor

Looks like @merrimanr commented on the email thread, so I'll copy it here for posterity.

I wrote the ReadMeUtils class a long time ago as a way to make documenting
the REST endpoints easier. The Controller class methods are annotated so
that endpoint documentation is displayed in Swagger but it is also
duplicated in the README. It seemed like a good idea at the time to
provide a utility to make this easier so that you only had to document in
one place. It was actually helpful (to me anyways) when we first
introduced a large number of REST endpoints and saved some tedious
copy/pasting.

In hindsight, there was no way of enforcing that we use the utility along
with the README.vm template. People intuitively edit the README.md
instead and the template quickly became stale. Eventually I got tired of
keeping the template in sync so I stopped using it as well. This class can
(and should) be safely removed.

I'd say just dump the template and the utility in this PR, since you'd already either have to clean it up or wait for another PR anyway.

@ottobackwards
Copy link
Contributor

The issue is that we should use this type of operation, but do it as part of maven no?

@nickwallen
Copy link
Contributor Author

The issue is that we should use this type of operation, but do it as part of maven no?

It doesn't seem that the utility is useful enough to maintain anymore. It had value once, but not any longer.

@ottobackwards
Copy link
Contributor

no, i mean auto documentation of the rest stuff ( and stellar stuff ) from maven builds is a good idea™

@ottobackwards
Copy link
Contributor

i'm not saying keep this utility

@merrimanr
Copy link
Contributor

My preference would be to just remove the endpoint documentation from the REST README and point users to Swagger where it's provided in context and easier to read. I see no value in duplicating that documentation.

@ottobackwards
Copy link
Contributor

The api should be documented without requiring the installation of the product.

@nickwallen
Copy link
Contributor Author

We can tackle the issue of documenting in a discuss thread. I'm not going to tackle that issue on this PR.

@ottobackwards
Copy link
Contributor

agreed, didn't meet to hijack.

my +1 stands

@nickwallen
Copy link
Contributor Author

No problem @ottobackwards . I just deleted the ReadMeUtils and README.vm files in the latest commit. I assume that whatever approach we take for documenting, we want to get rid of these files. If anyone disagrees with the latest commit, just let me know.

@cestella
Copy link
Member

+1 by inspection. Great job, @nickwallen

@anandsubbu
Copy link
Contributor

+1 (non-binding) @nickwallen . This is a much needed fix since it is now straight-forward to anyone new and wanting to try Metron.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants