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

NMS-13545: remove unnecessary fields from FlowSummary / FlowSummaryData #34

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

swachter
Copy link
Contributor

@swachter swachter commented Sep 1, 2021

@swachter swachter requested a review from fooker September 1, 2021 18:30
Copy link
Member

@fooker fooker left a comment

Choose a reason for hiding this comment

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

A bunch of questions:

  • This alters the output to elasticsearch, right?
  • Do we have an feedback from customers if there is any use of this data beside the queries done by OpenNMS itself?
  • Do we have a positive test run of the ONMS tests using the altered output format?

Comment on lines -69 to -71
private CompoundKeyType parent;
private RefType[] parts;
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary

@swachter
Copy link
Contributor Author

swachter commented Sep 14, 2021

A bunch of questions:

* This alters the output to elasticsearch, right?

Right.

* Do we have an feedback from customers if there is any use of this data beside the queries done by OpenNMS itself?

One customer is running this in production. How would the process be to get feedback from other customers?

* Do we have a positive test run of the ONMS tests using the altered output format?

The tests in OpenNMS still succeed.

@fooker
Copy link
Member

fooker commented Sep 14, 2021

@swachter

  • Do we have an feedback from customers if there is any use of this data beside the queries done by OpenNMS itself?

One customer is running this in production. How would the process be to get feedback from other customers?

Not sure. Asking the support team, maybe?

  • Do we have a positive test run of the ONMS tests using the altered output format?

The tests in OpenNMS still succeed.

Is there a CI run for this that I haven't found? or was this just done by a local run?
Both is fine as long as it covered the smoke tests (if there is any regarding nephron).

@swachter
Copy link
Contributor Author

@swachter

  • Do we have an feedback from customers if there is any use of this data beside the queries done by OpenNMS itself?

One customer is running this in production. How would the process be to get feedback from other customers?

Not sure. Asking the support team, maybe?

Will do.

  • Do we have a positive test run of the ONMS tests using the altered output format?

The tests in OpenNMS still succeed.

Is there a CI run for this that I haven't found? or was this just done by a local run?
Both is fine as long as it covered the smoke tests (if there is any regarding nephron).

It was a local run. There are no smoke tests for Nephron. All Nephron related tests are under features/flows/itests

Copy link
Member

@fooker fooker left a comment

Choose a reason for hiding this comment

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

Still has some unnecessary changes. beside that, this is good to merge.

@swachter
Copy link
Contributor Author

Support did not raise objections to dropping these fields.

@swachter swachter merged commit a23be3e into master Sep 16, 2021
@swachter swachter deleted the jira/NMS-13545 branch September 16, 2021 08:43
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.

2 participants