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

[#1936] Adapter Deletion: Show the Names of All Pipelines Using the Adapter and Allow One Click Deletion #2070

Merged
merged 30 commits into from
Oct 28, 2023

Conversation

muyangye
Copy link
Member

Purpose

See #1936

Remarks

PR introduces (a) breaking change(s): no

PR introduces (a) deprecation(s): no

@github-actions github-actions bot added java Pull requests that update Java code ui Anything that affects the UI backend Everything that is related to the StreamPipes backend labels Oct 22, 2023
@muyangye
Copy link
Member Author

Hi @bossenti @dominikriemer, I noticed the cypress test failure. However, my local tests did not fail so I am not able to reproduce the problem:
82e6b6890ad6e30248d6fe4fa0764c4
I then run the failed fileStream and machineData smoke tests and logged the delete-adapter button to the console and clearly from the screenshot below that button doesn't exist anymore once the adapter is deleted:
aea1a958809d2d784d93ce70f830fce
312193e11a7ec6d500306571a9600e7
Can you test on your machines locally too? And if those 2 tests failed, can you please give me some information so that I can fix the problem? Thank you!

@dominikriemer
Copy link
Member

Hi @muyangye great work! I think we just have some tests which currently randomly fail in CI.
Your PR looks good :-)

Copy link
Contributor

@tenthe tenthe left a comment

Choose a reason for hiding this comment

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

Hi @muyangye,

thank you very much for the PR. I tried it and it worked perfectly.

Cheers,
Philipp

# Conflicts:
#	streampipes-rest/src/main/java/org/apache/streampipes/rest/impl/connect/AdapterResource.java
@bossenti
Copy link
Contributor

If merged the recent changes from dev, PR can be merged if CI passes

@github-actions github-actions bot added the testing Relates to any kind of test (unit test, integration, or E2E test). label Oct 27, 2023
@muyangye
Copy link
Member Author

If merged the recent changes from dev, PR can be merged if CI passes

Hi @bossenti, I have tried multiple times but all cypress tests passed locally. The only way the 2 tests fail is when I set the timeout to a really low value such as 200. I see that the machine that runs CI is slower than my machine for every single test so I added more timeout. Can you trigger CI again? If it still doesn't pass then I sincerely don't understand what's wrong.

@muyangye
Copy link
Member Author

If it's not the problem of delay, then I guess it's probably because of the base url in cypress.config.ts is not reflective of the ui change since it's port is 80 whereas the port of built ui is 8082. This might cause the backend delete adapter to expect a boolean parameter (though I don't think it's a problem since there's a default false) whereas if the ui is not updated clicking delete will call deleteAdapter() without passing any boolean which causes the problem.

As you can see here, it's really guesses after guesses since local cypress tests succeed but CI cypress tests fail. Is it possible to enable full logs for cypress tests in CI😳?

@bossenti
Copy link
Contributor

Hey @muyangye,

Sorry for not mentioning it earlier: we have some troubles with flaky e2e tests in the recent days. So your PR is very likely not the reason for the failing tests.
I really appreciate your endeavors fixing the issue and I apologize for these inconvenience. Can you please revert your recent change affecting the base url?
I will then rerun the tests, if they still not pass we will take of this issue.

@dominikriemer
Copy link
Member

Yes, the UI by default runs on port 80 and 8082 is only used for development. So if you change back the cypress base url to port 80, this shoudl be ok!

@github-actions github-actions bot removed the testing Relates to any kind of test (unit test, integration, or E2E test). label Oct 28, 2023
@muyangye
Copy link
Member Author

Hey @muyangye,

Sorry for not mentioning it earlier: we have some troubles with flaky e2e tests in the recent days. So your PR is very likely not the reason for the failing tests. I really appreciate your endeavors fixing the issue and I apologize for these inconvenience. Can you please revert your recent change affecting the base url? I will then rerun the tests, if they still not pass we will take of this issue.

I see, I thought it's my PR's problem because I observed other PRs passing CI and my PR failing the 2 tests that involved delete adapter. Thanks for notifying me of this. I have reverted both the port and timeout changes🙂.

@bossenti
Copy link
Contributor

They sometimes pass and sometimes fail, let's cross fingers for this run 🤞
We need to fix the tests soon

@dominikriemer
Copy link
Member

dominikriemer commented Oct 28, 2023

I ran the failing tests locally without any problem so we can merge the PR once the current CI run is finished independent from its result.

@dominikriemer dominikriemer merged commit 03a416c into apache:dev Oct 28, 2023
17 of 18 checks passed
@bossenti bossenti linked an issue Nov 9, 2023 that may be closed by this pull request
3 tasks
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 java Pull requests that update Java code ui Anything that affects the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StreamPipes Connect: Ease deletion of adapters
4 participants