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.

Sign up for GitHub

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

Jump to bottom

Issue/1863 mysql sink quote fields #1865

Merged
merged 14 commits into from
Sep 17, 2020
Merged

Issue/1863 mysql sink quote fields #1865

merged 14 commits into from
Sep 17, 2020

Conversation

pmo-sdr
Copy link
Collaborator

@pmo-sdr pmo-sdr commented Apr 30, 2020

Fix for Issue #1863

@pmo-sdr pmo-sdr self-assigned this Apr 30, 2020
fgalan
fgalan reviewed Apr 30, 2020
View reviewed changes
cygnus-ngsi/src/main/java/com/telefonica/iot/cygnus/sinks/NGSIMySQLSink.java
@@ -338,7 +338,7 @@ protected NGSIGenericAggregator getAggregator(boolean rowAttrPersistence) {
private void persistAggregation(NGSIGenericAggregator aggregator)
Copy link
Member

Choose a reason for hiding this comment

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

An entry in CHANGES_NEXT_RELEASE describing the fix and citing #1863 should be included, using the usual style.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in 9010472

fgalan
fgalan approved these changes May 4, 2020
View reviewed changes
Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM

Passing the ball to other cygnuers for additional LGTM: @IvanHdzC @AlvaroVega @manucarrace

@dmartinezgomez
Extract Constant
32340f3 
In advance to  @IvanHdzC review ;)
@IvanHdzC
Copy link
Contributor

IvanHdzC commented May 4, 2020

Great @dmartinezgomez. In general I think everything is alright.

I just realized that those SQL methods have no tests so, can you please add a couple of tests for this overloaded method. It would be easier to add them to NGSIUtilsTest class, not directly to the Sink.

AlvaroVega
AlvaroVega approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

LGTM

@fgalan
Copy link
Member

fgalan commented Sep 7, 2020

I just realized that those SQL methods have no tests so, can you please add a couple of tests for this overloaded method. It would be easier to add them to NGSIUtilsTest class, not directly to the Sink.

I see recent activity in the PR (auther by @FranGomezCiC on August 27th)... should we understand that the above point has been covered by the new modifications?

@joelcamus
Copy link
Collaborator

I just realized that those SQL methods have no tests so, can you please add a couple of tests for this overloaded method. It would be easier to add them to NGSIUtilsTest class, not directly to the Sink.

I see recent activity in the PR (auther by @FranGomezCiC on August 27th)... should we understand that the above point has been covered by the new modifications?

We have added two tests for these SQL methods

@fgalan
Copy link
Member

fgalan commented Sep 10, 2020 •
edited
Loading

We have added two tests for these SQL methods

Great! Could you solve the conflict on CHANGES_NEXT_RELEASE so we can have a final look and eventually merge, please?

IvanHdzC
IvanHdzC reviewed Sep 11, 2020
View reviewed changes
Copy link
Contributor

@IvanHdzC IvanHdzC left a comment

Choose a reason for hiding this comment

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

Please update your branch with current master before solving merge conflicts.

@joelcamus
Copy link
Collaborator

We have added two tests for these SQL methods

Great! Could you solve the conflict on CHANGES_NEXT_RELEASE so we can have a final look and eventually merge, please?

Done

@joelcamus
Copy link
Collaborator

Please update your branch with current master before solving merge conflicts.

Done

@joelcamus
Copy link
Collaborator

I apologise for the late

IvanHdzC
IvanHdzC approved these changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@IvanHdzC IvanHdzC left a comment

Choose a reason for hiding this comment

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

LGTM

@fgalan fgalan merged commit 07b54a5 into telefonicaid:master Sep 17, 2020
@fgalan
Copy link
Member

fgalan commented Sep 17, 2020

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelcamus joelcamus joelcamus left review comments

@fgalan fgalan fgalan approved these changes

@IvanHdzC IvanHdzC IvanHdzC approved these changes

@AlvaroVega AlvaroVega AlvaroVega approved these changes

Assignees

@pmo-sdr pmo-sdr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pmo-sdr @IvanHdzC @fgalan @joelcamus @AlvaroVega @dmartinezgomez @FranGomezCiC