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

#2144 Enhance Static Properties by UI validation V2 #2443

Merged
merged 23 commits into from
Mar 6, 2024
Merged

#2144 Enhance Static Properties by UI validation V2 #2443

merged 23 commits into from
Mar 6, 2024

Conversation

tenthe
Copy link
Contributor

@tenthe tenthe commented Jan 29, 2024

Purpose

Remarks

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

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

@github-actions github-actions bot added java Pull requests that update Java code connect Related to the `connect` module (adapters) backend Everything that is related to the StreamPipes backend testing Relates to any kind of test (unit test, integration, or E2E test). labels Jan 29, 2024
@tenthe
Copy link
Contributor Author

tenthe commented Jan 29, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor Author

tenthe commented Jan 30, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor Author

tenthe commented Feb 2, 2024

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

@IsaakKrut
Copy link
Contributor

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

#2454

Copy link
Contributor

Hello there 👋

We noticed that it's been some time since activity occurred on your pull request 🤔. 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 ⏰.

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

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

@github-actions github-actions bot added the stale Marks pull requests that are classified as `stale` by our bot. label Feb 25, 2024
@IsaakKrut
Copy link
Contributor

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

@github-actions github-actions bot removed the stale Marks pull requests that are classified as `stale` by our bot. label Feb 27, 2024
@tenthe
Copy link
Contributor Author

tenthe commented Feb 29, 2024

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

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Mar 4, 2024
@tenthe
Copy link
Contributor Author

tenthe commented Mar 4, 2024

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

@dominikriemer dominikriemer self-requested a review March 4, 2024 16:12
@IsaakKrut
Copy link
Contributor

Hi @tenthe,

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

Copy link
Member

@dominikriemer dominikriemer left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks a lot @IsaakKrut and sorry that this took so long!

@tenthe tenthe merged commit 9cf1473 into dev Mar 6, 2024
18 checks passed
@tenthe tenthe deleted the fork/dev branch March 6, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Everything that is related to the StreamPipes backend connect Related to the `connect` module (adapters) dependencies Pull requests that update a dependency file java Pull requests that update Java code testing Relates to any kind of test (unit test, integration, or E2E test).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants