apache / streampipes

Apache StreamPipes - A self-service (Industrial) IoT toolbox to enable non-technical users to connect, analyze and explore IoT data streams.
https://streampipes.apache.org
Apache License 2.0
566 stars 174 forks source link

#2144 Enhance Static Properties by UI validation V2 #2443

Closed tenthe closed 3 months ago

tenthe commented 4 months ago

Purpose

Remarks

PR introduces (a) breaking change(s): <yes/no>

PR introduces (a) deprecation(s): <yes/no>

tenthe commented 4 months ago

@IsaakKrut, there seems to be an issue with the unit test. Do you have an idea what is going wrong. I tried to mock the extractor but it did not work. Cheers, Philipp

IsaakKrut commented 4 months ago

@tenthe The 3 values(serverAddressValue, hostValue, portValue) from the extractor you are using in OpcUaAdapterMigrationV2 have to be present in the extractor. Without them I'm getting NullPointerException in the AbstractParameterExtractor.singleValueParameter method call. Could we instantiate an extractor with those properties inside the test? And if so, are they gonna be available during actual migration as well?

tenthe commented 4 months ago

@IsaakKrut, I tested the migration and there I did not get any exceptions. I only geth the NullPointer when running the test. Maybe we can mock the extractor return a value when the function is called with the parameters (serverAddressValue, hostValue, portValue). Do you think this will work?

IsaakKrut commented 4 months ago

@tenthe All these 3 fields (serverAddressValue, hostValue, portValue) are going to have a value during migration, right? If so we can add the defaultValue parameter for host and port for version 2 config in OpcUaAdapterVersionedConfig on lines 138, 140. And create an extractor inside the test with the same default values. Ran the test with those changes and it passed image

image

tenthe commented 4 months ago

Ok, great. Can you add your changes. Then we can close this PR.

IsaakKrut commented 4 months ago

Created a separate PR to update this branch since I don't have push access

2454

github-actions[bot] commented 3 months ago

Hello there :wave:

We noticed that it's been some time since activity occurred on your pull request :thinking:. In order to keep things moving forward, we're marking this PR as stale and giving you 7 days to respond before it's automatically closed :alarm_clock:.

Please take a moment to review your pull request and make any necessary updates or changes :man_technologist:. If you need more time or have any questions, please don't hesitate to let us know :speech_balloon:.

Thank you for your contributions to our project, and we look forward to hearing back from you soon :pray:.

IsaakKrut commented 3 months ago

Hi @tenthe, did you have a chance to look at PR #2454?

tenthe commented 3 months ago

Hi @IsaakKrut, sorry for the late reply. I'll have a look at it in the upcominig days. Cheers, Philipp

tenthe commented 3 months ago

Hi @IsaakKrut,

I have looked through the changes.

So far I have assumed that we have to create a new static property. But since only the datantype in the description of the FreeTextStaticProperty changes and not the runtime value, I found a simpler solution.

I have adapted code acordingly. What do you think about the changes?

Cheers, Philipp

IsaakKrut commented 3 months ago

Hi @tenthe,

Thanks for the update, looks much cleaner. Both the implementation and the tests. I tested locally and it works as expected