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

Add webhooks to test framework snapshot #11281

Merged
merged 7 commits into from
May 13, 2022
Merged

Add webhooks to test framework snapshot #11281

merged 7 commits into from
May 13, 2022

Conversation

frankbu
Copy link
Collaborator

@frankbu frankbu commented May 9, 2022

Please provide a description for what this PR is for.

And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.

  • Configuration Infrastructure
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Policies and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

@frankbu frankbu requested review from a team as code owners May 9, 2022 20:58
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 9, 2022
@frankbu frankbu mentioned this pull request May 9, 2022
10 tasks
@frankbu frankbu requested a review from a team as a code owner May 11, 2022 21:04
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 12, 2022
@ericvn
Copy link
Contributor

ericvn commented May 13, 2022

Thoughts on if just adding additional instructions in the various test scripts is valid here? This implies that we know that the cleanup steps within the various doc pages are incorrect as things are not getting cleaned up correctly.

At one point, we might have found something left behind and the updated the index.md to fix the missed items. This flows through to the snips and if the test uses the snippet (it should), we have something that is testing the page.

Adding the uninstall directly in the script would prevent this type of thing from happening going forward, unless of course the uninstall is broken and not removing something (which may be why we have other kubectl commands deleting things).

We seem to be ignoring that the tests are validating what each page mentions.

@frankbu
Copy link
Collaborator Author

frankbu commented May 13, 2022

The cleanup instructions in the various docs are a mix. Some do complete uninstall, some do partial, and some have no cleanup instructions at all. The ones that have partial or nothing have uninstall instructions in the test.sh, but many of those are only partially correct. Bottom line is that we need figure out what should be the "correct" way to cleanup (might require fixes in istioctl commands also), and then we can do a pass through the docs/tests to do a better job, but that is out of scope for this PR which is just adding more to the before/after snapshot checking and patching up the broken tests (consistently with how they are currently written). There are some comments in the code mentioning some specific TODOs for cleaning up tests.

Copy link
Contributor

@ericvn ericvn left a comment

Choose a reason for hiding this comment

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

It looks like my comment on cleanup and tests using what is in the docs is only happening for 3 of the changed tests, two cert-management and another. All the other tests are doing random cleanup in the script outside of the doc.

So while my concern is correct, it seems that we are already doing something else for a bunch of tests (which we should clean up later as mentioned by Frank).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants